From 4da8d1d774988a7d8cdd14356179d42e4685986e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 26 May 2026 07:51:12 -0400 Subject: [PATCH 1/5] =?UTF-8?q?fix(importer):=20race-safe=20savepoint-base?= =?UTF-8?q?d=20find-or-create=20for=20Source=20+=20Post=20(uq=5Fsource=5Fa?= =?UTF-8?q?rtist=5Fplatform=5Furl=20UniqueViolation=20operator-flagged=202?= =?UTF-8?q?026-05-26)=20=E2=80=94=20Co-Authored-By:=20Claude=20Opus=204.7?= =?UTF-8?q?=20(1M=20context)=20?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- backend/app/services/importer.py | 130 +++++++++++++++-------- tests/test_importer_upsert_helpers.py | 147 ++++++++++++++++++++++++++ 2 files changed, 234 insertions(+), 43 deletions(-) create mode 100644 tests/test_importer_upsert_helpers.py diff --git a/backend/app/services/importer.py b/backend/app/services/importer.py index fb9d0e3..3223ec7 100644 --- a/backend/app/services/importer.py +++ b/backend/app/services/importer.py @@ -18,6 +18,7 @@ from pathlib import Path from PIL import Image from sqlalchemy import select +from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session from ..models import ( @@ -202,6 +203,80 @@ class Importer: (phash, width or 0, height or 0, image_id) ) + def _find_or_create_source( + self, *, artist_id: int, platform: str, url: str, + ) -> Source: + """Race-safe find-or-create on `source` keyed by + (artist_id, platform, url) — the same key as the + `uq_source_artist_platform_url` constraint. + + Two concurrent workers processing different files in the same + post can both find no existing Source row then both INSERT, + which trips the unique constraint and poisons the session with + `psycopg.errors.UniqueViolation`. Operator-flagged 2026-05-26. + + Pattern: select; if absent, open a savepoint and INSERT. + On IntegrityError, roll the savepoint back (NOT the outer + transaction, which would lose the surrounding scan's progress) + and re-select — the concurrent op just created the row we + wanted, so the second select will find it. + """ + existing = self.session.execute( + select(Source).where( + Source.artist_id == artist_id, + Source.platform == platform, + Source.url == url, + ) + ).scalar_one_or_none() + if existing is not None: + return existing + sp = self.session.begin_nested() + try: + row = Source(artist_id=artist_id, platform=platform, url=url) + self.session.add(row) + self.session.flush() + sp.commit() + return row + except IntegrityError: + sp.rollback() + return self.session.execute( + select(Source).where( + Source.artist_id == artist_id, + Source.platform == platform, + Source.url == url, + ) + ).scalar_one() + + def _find_or_create_post( + self, *, source_id: int, external_post_id: str, + ) -> Post: + """Race-safe find-or-create on `post` keyed by + (source_id, external_post_id). Mirrors `_find_or_create_source` + — same savepoint + IntegrityError-recovery pattern.""" + existing = self.session.execute( + select(Post).where( + Post.source_id == source_id, + Post.external_post_id == external_post_id, + ) + ).scalar_one_or_none() + if existing is not None: + return existing + sp = self.session.begin_nested() + try: + row = Post(source_id=source_id, external_post_id=external_post_id) + self.session.add(row) + self.session.flush() + sp.commit() + return row + except IntegrityError: + sp.rollback() + return self.session.execute( + select(Post).where( + Post.source_id == source_id, + Post.external_post_id == external_post_id, + ) + ).scalar_one() + def import_one(self, source: Path) -> ImportResult: """Dispatch by kind. Media → normal pipeline. Archive → extract media members (one Post via the archive-adjacent sidecar) and @@ -241,29 +316,13 @@ class Importer: sd = parse_sidecar(data) platform = sd.platform or "unknown" url = sd.post_url or f"sidecar:{platform}" - src = self.session.execute( - select(Source).where( - Source.artist_id == artist.id, - Source.platform == platform, - Source.url == url, - ) - ).scalar_one_or_none() - if src is None: - src = Source(artist_id=artist.id, platform=platform, url=url) - self.session.add(src) - self.session.flush() + src = self._find_or_create_source( + artist_id=artist.id, platform=platform, url=url, + ) epid = sd.external_post_id or sc.stem - post = self.session.execute( - select(Post).where( - Post.source_id == src.id, - Post.external_post_id == epid, - ) - ).scalar_one_or_none() - if post is None: - post = Post(source_id=src.id, external_post_id=epid) - self.session.add(post) - self.session.flush() - return post + return self._find_or_create_post( + source_id=src.id, external_post_id=epid, + ) def _capture_attachment( self, source: Path, *, post: Post | None = None, @@ -705,29 +764,14 @@ class Importer: else: platform = sd.platform or "unknown" url = sd.post_url or f"sidecar:{platform}" - src = self.session.execute( - select(Source).where( - Source.artist_id == artist.id, - Source.platform == platform, - Source.url == url, - ) - ).scalar_one_or_none() - if src is None: - src = Source(artist_id=artist.id, platform=platform, url=url) - self.session.add(src) - self.session.flush() + src = self._find_or_create_source( + artist_id=artist.id, platform=platform, url=url, + ) epid = sd.external_post_id or sc.stem - post = self.session.execute( - select(Post).where( - Post.source_id == src.id, - Post.external_post_id == epid, - ) - ).scalar_one_or_none() - if post is None: - post = Post(source_id=src.id, external_post_id=epid) - self.session.add(post) - self.session.flush() + post = self._find_or_create_post( + source_id=src.id, external_post_id=epid, + ) if sd.post_url is not None: post.post_url = sd.post_url if sd.post_title is not None: diff --git a/tests/test_importer_upsert_helpers.py b/tests/test_importer_upsert_helpers.py new file mode 100644 index 0000000..ca8a3bc --- /dev/null +++ b/tests/test_importer_upsert_helpers.py @@ -0,0 +1,147 @@ +"""Tests for Importer._find_or_create_source / _find_or_create_post — +the race-safe savepoint-based helpers that replaced the previous +check-then-insert pattern. + +Operator-flagged 2026-05-26: concurrent workers processing different +files in the same post both found no existing Source row, then both +INSERTed, tripping uq_source_artist_platform_url and poisoning the +session with `psycopg.errors.UniqueViolation`. The new helpers wrap +the INSERT in a savepoint and recover from IntegrityError by +re-selecting the row that the concurrent op committed. + +Tests cover: + - idempotent return: same (artist_id, platform, url) → same Source row + - idempotent return for Post: same (source_id, external_post_id) → same Post + - IntegrityError recovery: monkeypatched flush raises once, helper + finds the row a concurrent op committed +""" + +from pathlib import Path + +import pytest +from sqlalchemy import select +from sqlalchemy.exc import IntegrityError + +from backend.app.models import Artist, ImportSettings, Post, Source +from backend.app.services.importer import Importer +from backend.app.services.thumbnailer import Thumbnailer + +pytestmark = pytest.mark.integration + + +@pytest.fixture +def importer(db_sync, tmp_path): + import_root = tmp_path / "import" + images_root = tmp_path / "images" + import_root.mkdir() + images_root.mkdir() + settings = db_sync.execute( + select(ImportSettings).where(ImportSettings.id == 1) + ).scalar_one() + return Importer( + session=db_sync, + images_root=images_root, + import_root=import_root, + thumbnailer=Thumbnailer(images_root=images_root), + settings=settings, + ) + + +@pytest.fixture +def artist_row(db_sync): + a = Artist(name="TestArtist", slug="testartist") + db_sync.add(a) + db_sync.flush() + return a + + +def test_find_or_create_source_creates_then_returns_existing(importer, artist_row): + s1 = importer._find_or_create_source( + artist_id=artist_row.id, platform="patreon", + url="https://www.patreon.com/posts/test-1", + ) + s2 = importer._find_or_create_source( + artist_id=artist_row.id, platform="patreon", + url="https://www.patreon.com/posts/test-1", + ) + assert s1.id == s2.id + + +def test_find_or_create_source_distinct_urls_yield_distinct_rows( + importer, artist_row, +): + a = importer._find_or_create_source( + artist_id=artist_row.id, platform="patreon", + url="https://www.patreon.com/posts/a", + ) + b = importer._find_or_create_source( + artist_id=artist_row.id, platform="patreon", + url="https://www.patreon.com/posts/b", + ) + assert a.id != b.id + + +def test_find_or_create_post_idempotent(importer, artist_row, db_sync): + src = importer._find_or_create_source( + artist_id=artist_row.id, platform="patreon", + url="https://www.patreon.com/posts/post-test", + ) + p1 = importer._find_or_create_post( + source_id=src.id, external_post_id="ext-001", + ) + p2 = importer._find_or_create_post( + source_id=src.id, external_post_id="ext-001", + ) + assert p1.id == p2.id + + +def test_find_or_create_source_recovers_from_integrity_error( + importer, artist_row, db_sync, monkeypatch, +): + """Simulate the race: another worker has already inserted a Source row + matching our (artist_id, platform, url) just before our flush would + have. Our flush raises IntegrityError; the helper rolls back the + savepoint and re-selects, returning the row the concurrent op created. + """ + canonical_url = "https://www.patreon.com/posts/race-141226276" + pre_existing = Source( + artist_id=artist_row.id, platform="patreon", url=canonical_url, + ) + db_sync.add(pre_existing) + db_sync.flush() + + # Force a fresh select within the helper to MISS the existing row by + # detaching it from the identity map; SQLAlchemy's first-level cache + # would otherwise return the pre_existing row immediately. + # Easier: monkeypatch the FIRST select inside the helper to return + # None on first call, real result on subsequent. We do that by + # patching session.execute with a single-shot wrapper. + real_execute = db_sync.execute + skip_count = [0] + + def execute_with_first_select_miss(stmt, *args, **kwargs): + # Strip-down heuristic: the first SELECT issued by the helper is + # the existence check. Force it to return a "no row" result. + result = real_execute(stmt, *args, **kwargs) + if skip_count[0] == 0: + skip_count[0] += 1 + # Wrap result so .scalar_one_or_none() returns None for this + # one call, then unwrap on subsequent uses. + class _ForcedMiss: + def scalar_one_or_none(self): + return None + + def scalar_one(self): + return result.scalar_one() + + def __getattr__(self, name): + return getattr(result, name) + return _ForcedMiss() + return result + + monkeypatch.setattr(db_sync, "execute", execute_with_first_select_miss) + + recovered = importer._find_or_create_source( + artist_id=artist_row.id, platform="patreon", url=canonical_url, + ) + assert recovered.id == pre_existing.id From ebd985990c1a38e4b020949d4e51a0856c4955f3 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 26 May 2026 07:53:14 -0400 Subject: [PATCH 2/5] =?UTF-8?q?feat(ui):=20ErrorDetailModal=20=E2=80=94=20?= =?UTF-8?q?click=20error=20=E2=86=92=20flat-text=20modal=20with=20copy=20b?= =?UTF-8?q?utton=20(replaces=20unusable=20:title=20tooltip=20for=20multi-l?= =?UTF-8?q?ine=20SQLAlchemy=20tracebacks)=20=E2=80=94=20Co-Authored-By:=20?= =?UTF-8?q?Claude=20Opus=204.7=20(1M=20context)=20?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../components/common/ErrorDetailModal.vue | 87 +++++++++++++++++++ .../components/settings/ImportTaskList.vue | 45 +++++++++- .../components/settings/SystemActivityTab.vue | 42 ++++++++- 3 files changed, 169 insertions(+), 5 deletions(-) create mode 100644 frontend/src/components/common/ErrorDetailModal.vue diff --git a/frontend/src/components/common/ErrorDetailModal.vue b/frontend/src/components/common/ErrorDetailModal.vue new file mode 100644 index 0000000..d8281a5 --- /dev/null +++ b/frontend/src/components/common/ErrorDetailModal.vue @@ -0,0 +1,87 @@ + + + + + diff --git a/frontend/src/components/settings/ImportTaskList.vue b/frontend/src/components/settings/ImportTaskList.vue index c3edbd1..97bbebf 100644 --- a/frontend/src/components/settings/ImportTaskList.vue +++ b/frontend/src/components/settings/ImportTaskList.vue @@ -45,9 +45,11 @@
@@ -100,16 +102,35 @@ + + + + diff --git a/frontend/src/components/settings/SystemActivityTab.vue b/frontend/src/components/settings/SystemActivityTab.vue index 1db2724..f623bb0 100644 --- a/frontend/src/components/settings/SystemActivityTab.vue +++ b/frontend/src/components/settings/SystemActivityTab.vue @@ -59,8 +59,12 @@ {{ r.queue }} {{ shortTaskName(r.task_name) }} {{ r.target_id ?? '—' }} - - {{ r.error_type }} + + @@ -138,6 +142,12 @@
+ + @@ -145,8 +155,23 @@ import { computed, onMounted, onUnmounted, ref } from 'vue' import { useSystemActivityStore } from '../../stores/systemActivity.js' +import ErrorDetailModal from '../common/ErrorDetailModal.vue' import QueuesTable from './QueuesTable.vue' +// Click-to-open modal for full error text. Replaces the unusable +// :title="..." tooltip (operator-flagged 2026-05-26: SQLAlchemy +// rollback + traceback content rendered as a cramped browser tooltip +// you couldn't copy from or scroll within). +const showErrorModal = ref(false) +const errorModalTitle = ref('') +const errorModalMessage = ref('') + +function openError(title, message) { + errorModalTitle.value = title || 'Error details' + errorModalMessage.value = message || '' + showErrorModal.value = true +} + const store = useSystemActivityStore() const filterQueue = ref(null) @@ -276,5 +301,18 @@ function formatRelative(iso) { font-feature-settings: 'tnum'; } .fc-err { color: rgb(var(--v-theme-error, 220 80 80)); } +.fc-err-link { + /* Styled as a text-only button so the error_type cell stays + visually identical to the prior tooltip-bearing row, but is + now a real clickable target with hover affordance. */ + color: rgb(var(--v-theme-error, 220 80 80)); + background: transparent; + border: 0; + padding: 0; + font: inherit; + text-decoration: underline dotted; + cursor: pointer; +} +.fc-err-link:hover { text-decoration: underline; } .fc-muted { color: rgb(var(--v-theme-on-surface-variant)); } From a06ada4c9b6a03a1697c65ddf2f64cd083af81af Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 26 May 2026 08:07:22 -0400 Subject: [PATCH 3/5] =?UTF-8?q?fix(ext-ui):=20direct=20:href=20install=20b?= =?UTF-8?q?utton=20(Firefox=20needs=20anchor=20click,=20not=20programmatic?= =?UTF-8?q?=20navigation)=20+=20manifest=20version=20detection=20ignores?= =?UTF-8?q?=20-latest.xpi=20alias=20=E2=80=94=20Co-Authored-By:=20Claude?= =?UTF-8?q?=20Opus=204.7=20(1M=20context)=20?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- backend/app/api/extension.py | 16 +++++++++++++--- .../components/settings/BrowserExtensionCard.vue | 14 ++++++++------ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/backend/app/api/extension.py b/backend/app/api/extension.py index 54d569f..4f4766f 100644 --- a/backend/app/api/extension.py +++ b/backend/app/api/extension.py @@ -93,10 +93,20 @@ def _read_manifest_sync() -> dict | None: asyncio.to_thread (ASYNC240: no pathlib I/O in async functions).""" if not XPI_DIR.is_dir(): return None - xpis = sorted(XPI_DIR.glob("fabledcurator-*.xpi"), key=lambda p: p.stat().st_mtime) - if not xpis: + # Exclude the `fabledcurator-latest.xpi` alias when picking the file to + # extract a version from — it's a copy of the latest versioned XPI, + # written at the same mtime by build.yml, and would otherwise tie or + # win the sort (operator-flagged 2026-05-26: UI displayed "v latest" + # because `_extract_version("fabledcurator-latest.xpi")` returns + # the literal "latest"). The alias still serves as `latest_url`. + versioned = [ + p for p in XPI_DIR.glob("fabledcurator-*.xpi") + if p.name != "fabledcurator-latest.xpi" + ] + if not versioned: return None - latest = xpis[-1] + versioned.sort(key=lambda p: p.stat().st_mtime) + latest = versioned[-1] return { "installed": True, "version": _extract_version(latest.name), diff --git a/frontend/src/components/settings/BrowserExtensionCard.vue b/frontend/src/components/settings/BrowserExtensionCard.vue index 2dbe591..db5d3a3 100644 --- a/frontend/src/components/settings/BrowserExtensionCard.vue +++ b/frontend/src/components/settings/BrowserExtensionCard.vue @@ -34,11 +34,18 @@ @@ -25,7 +27,6 @@ import MLThresholdSliders from './MLThresholdSliders.vue' import AllowlistTable from './AllowlistTable.vue' import AliasTable from './AliasTable.vue' import BackupCard from './BackupCard.vue' -import TagMaintenanceCard from './TagMaintenanceCard.vue' import LegacyMigrationCard from './LegacyMigrationCard.vue' diff --git a/frontend/src/stores/cleanup.js b/frontend/src/stores/cleanup.js new file mode 100644 index 0000000..fac5112 --- /dev/null +++ b/frontend/src/stores/cleanup.js @@ -0,0 +1,72 @@ +import { defineStore } from 'pinia' +import { ref } from 'vue' +import { useApi } from '../composables/useApi.js' + +export const useCleanupStore = defineStore('cleanup', () => { + const api = useApi() + + // Defaults sourced from ImportSettings on mount. Cards pre-fill from + // these so the common case ("apply current import filters + // retroactively") is one click; operator can override per-audit. + const defaults = ref({ + min_width: 0, + min_height: 0, + transparency_threshold: 0.9, + single_color_threshold: 0.95, + single_color_tolerance: 30, + }) + + const recentRuns = ref([]) + + async function loadDefaults() { + const s = await api.get('/api/settings/import') + defaults.value = { + min_width: s.min_width ?? 0, + min_height: s.min_height ?? 0, + transparency_threshold: s.transparency_threshold ?? 0.9, + single_color_threshold: s.single_color_threshold ?? 0.95, + single_color_tolerance: s.single_color_tolerance ?? 30, + } + } + + async function previewMinDim(min_width, min_height) { + return await api.post('/api/cleanup/min-dimension/preview', { + body: { min_width, min_height }, + }) + } + + async function deleteMinDim(min_width, min_height, confirm) { + return await api.post('/api/cleanup/min-dimension/delete', { + body: { min_width, min_height, confirm }, + }) + } + + async function startAudit(rule, params) { + return await api.post('/api/cleanup/audit', { body: { rule, params } }) + } + + async function getAudit(id) { + return await api.get(`/api/cleanup/audit/${id}`) + } + + async function loadHistory(limit = 20) { + const body = await api.get(`/api/cleanup/audit?limit=${limit}`) + recentRuns.value = body.runs + return body.runs + } + + async function applyAudit(id, confirm) { + return await api.post(`/api/cleanup/audit/${id}/apply`, { body: { confirm } }) + } + + async function cancelAudit(id) { + return await api.post(`/api/cleanup/audit/${id}/cancel`) + } + + return { + defaults, recentRuns, + loadDefaults, + previewMinDim, deleteMinDim, + startAudit, getAudit, loadHistory, applyAudit, cancelAudit, + } +}) diff --git a/frontend/src/views/CleanupView.vue b/frontend/src/views/CleanupView.vue new file mode 100644 index 0000000..f02e63a --- /dev/null +++ b/frontend/src/views/CleanupView.vue @@ -0,0 +1,31 @@ + + + + + diff --git a/frontend/src/views/SettingsView.vue b/frontend/src/views/SettingsView.vue index 8d41770..99f536d 100644 --- a/frontend/src/views/SettingsView.vue +++ b/frontend/src/views/SettingsView.vue @@ -14,6 +14,7 @@ Overview Activity Import + Cleanup Maintenance @@ -53,6 +54,10 @@ + + + + @@ -72,6 +77,7 @@ import ImportTriggerPanel from '../components/settings/ImportTriggerPanel.vue' import ImportFiltersForm from '../components/settings/ImportFiltersForm.vue' import ImportTaskList from '../components/settings/ImportTaskList.vue' import MaintenancePanel from '../components/settings/MaintenancePanel.vue' +import CleanupView from './CleanupView.vue' import { useMLStore } from '../stores/ml.js' const tab = ref('overview') diff --git a/tests/test_api_cleanup.py b/tests/test_api_cleanup.py index 2f48960..ba9534f 100644 --- a/tests/test_api_cleanup.py +++ b/tests/test_api_cleanup.py @@ -150,7 +150,7 @@ async def test_audit_get_by_id_returns_full_row(client, db): @pytest.mark.asyncio async def test_audit_history_returns_recent_runs(client, db): - for i in range(3): + for _ in range(3): db.add(LibraryAuditRun( rule="transparency", params={"threshold": 0.9}, status="applied", matched_ids=[],