audit-g5: architectural debt — 4 bundles (A/B/C/D) #53

Merged

bvandeusen merged 6 commits from dev into main 2026-06-02 18:07:25 -04:00

Conversation 0 Commits 6 Files Changed 22

+303 -67

bvandeusen commented 2026-06-02 18:07:19 -04:00

Owner

Copy Link

Copy Source

Summary

Group 5 of the 2026-06-02 multi-system drift audit. Four sub-bundles of architectural debt fixes:

Bundle A — small cleanups (75c63e1)

• auth_type_for replaces hardcoded ('discord','pixiv') tuple in download_service — a 7th token-platform now picks the right credential path automatically.
• Tag merge enqueues recompute_centroid.delay so the target's centroid reflects its new image set immediately (was: 24h drift window via daily list_drifted).
• backfill_thumbnails finally on beat_schedule (daily) — task docstring claimed periodic Beat but the entry was never registered; library got no self-healing thumbnail repair.
• modal.createAndAdd pushes a kind='fandom' tag into tagsStore.fandomCache so FandomPicker sees it on next open instead of needing a full page reload.
• Cleanup cluster:
  • Dropped .webp from cleanup_service.unlink (thumbnailer only writes .jpg/.png)
  • Dropped effective_date from /api/gallery/scroll response (no FE consumer reads it)
  • Renamed store.recentMinute → store.recentRuns across 3 consumers (data is last-200-runs, not last-60s; the name lied)

Bundle B — race-poisoning in 3 sites (8d75ade)

Three SELECT-then-INSERT sites that lost the race under concurrent writers / recovery-sweep replays, poisoning the outer transaction with an unrecoverable IntegrityError. All three now use begin_nested savepoint + IntegrityError rollback + re-SELECT, mirroring importer._get_or_create.

• importer._capture_attachment (PostAttachment.sha256 UNIQUE)
• TagService.find_or_create (partial uniqueness on (name, kind, COALESCE(fandom_id, -1)))
• ArtistService.find_or_create (had IntegrityError handling but used session.rollback() — now savepoint, so the surrounding request's progress isn't lost)

Bundle C — Fernet key safety on partial restore (4df9817 + conftest fix at 9f7261b)

CredentialCrypto._load_or_create_key was silently minting a new Fernet key whenever the file was missing. Failure mode the audit flagged: partial restore where DB was restored but /images/secrets/ was lost → working-looking system where every authenticated download fails AUTH_ERROR until operator re-uploads every credential by hand.

Two opt-ins now required for auto-create:

1. Explicit bootstrap_ok=True kwarg (tests, scripts), or
2. CURATOR_BOOTSTRAP_NEW_KEY=1 env var (operator first-time-setup opt-in)

Otherwise raises MissingCredentialKey so the app fails fast at startup. Loud log on key creation. Docstring corrected (was: "images/data root" / actual: /images/secrets/credential_key.b64).

tests/conftest.py sets the env var at module load so existing test suites work unchanged.

Operator note: if your running container has /images/secrets/credential_key.b64 already present, no behavior change. If somehow the file is missing on the next deploy, the app refuses to start with a clear message telling the operator to restore from backup OR set the env var.

Bundle D — ErrorType chip on FailingSourcesCard (f05aaa7)

Alembic 0032 adds Source.error_type (varchar(32), indexed). _update_source_health stamps it on status='error' and clears on 'ok'. SourceRecord.to_dict exposes it.

FailingSourcesCard.vue renders a colored chip next to the consecutive-failures count, with a tooltip explaining the suggested operator action for each ErrorType category. Color reflects triage intent:

• warning (yellow) — operator action needed (auth_error)
• info (blue) — backend-paced (rate_limited / timeout / network_error / partial / tier_limited)
• error (red) — likely terminal without intervention (not_found / access_denied / validation_failed / unsupported_url / http_error / unknown_error)

The audit flagged this: backend computed 13 ErrorType categories but only the free-text last_error reached the operator. Bulk-triage by class ("all auth_error → rotate cookies", "12 rate_limited → just wait") required opening Logs per row.

Test Plan

• Add a credentialed source on a non-(discord,pixiv) token platform if one ever lands — confirm auth_type_for routes the credential correctly
• Merge two tags via the directory UI — confirm a recompute_centroid task fires (visible in System Activity)
• Wait for the daily backfill_thumbnails Beat tick or trigger manually — confirm library missing-thumbnail rows are repaired
• Create a new fandom tag from the modal kind:fandom flow — confirm FandomPicker shows it without a page reload
• Stop a worker mid-download to force a recovery sweep on a fresh artist — confirm no IntegrityError poisons the request
• Verify that /images/secrets/credential_key.b64 exists in your deployed container before this rolls out (so the app boot doesn't trip the new safety check)
• Trigger a failing source on each ErrorType class — confirm the chip color matches the triage intent

Pending in Group 5 (not in this PR)

• G5.1 — Centroid version two-sources-of-truth (env SIGLIP_VERSION vs MLSettings.embedder_model_version): needs design decision before fixing.
• G5.4 — Modal cross-route navigation leaks: needs UX behavior choice (RouterLink in modal closes vs preserves? ?image=N strips on close?).
• Duplicate tag-merge endpoint (/api/tags vs /api/admin/tags) — has 1 FE caller + 3 tests on the admin path; deserves its own consolidation change.

Audit reference

This closes Bundles A/B/C/D of Group 5 (plan task Scribe #551). After merge, G5.1 + G5.4 are the remaining open findings.

🤖 Generated with Claude Code

## Summary Group 5 of the 2026-06-02 multi-system drift audit. Four sub-bundles of architectural debt fixes: ### Bundle A — small cleanups (75c63e1) - **`auth_type_for`** replaces hardcoded `('discord','pixiv')` tuple in `download_service` — a 7th token-platform now picks the right credential path automatically. - **Tag merge enqueues `recompute_centroid.delay`** so the target's centroid reflects its new image set immediately (was: 24h drift window via daily `list_drifted`). - **`backfill_thumbnails` finally on `beat_schedule`** (daily) — task docstring claimed periodic Beat but the entry was never registered; library got no self-healing thumbnail repair. - **`modal.createAndAdd` pushes a kind=`'fandom'` tag into `tagsStore.fandomCache`** so FandomPicker sees it on next open instead of needing a full page reload. - **Cleanup cluster:** - Dropped `.webp` from `cleanup_service.unlink` (thumbnailer only writes .jpg/.png) - Dropped `effective_date` from `/api/gallery/scroll` response (no FE consumer reads it) - Renamed `store.recentMinute` → `store.recentRuns` across 3 consumers (data is last-200-runs, not last-60s; the name lied) ### Bundle B — race-poisoning in 3 sites (8d75ade) Three SELECT-then-INSERT sites that lost the race under concurrent writers / recovery-sweep replays, poisoning the outer transaction with an unrecoverable `IntegrityError`. All three now use `begin_nested` savepoint + IntegrityError rollback + re-SELECT, mirroring `importer._get_or_create`. - `importer._capture_attachment` (`PostAttachment.sha256` UNIQUE) - `TagService.find_or_create` (partial uniqueness on `(name, kind, COALESCE(fandom_id, -1))`) - `ArtistService.find_or_create` (had IntegrityError handling but used `session.rollback()` — now savepoint, so the surrounding request's progress isn't lost) ### Bundle C — Fernet key safety on partial restore (4df9817 + conftest fix at 9f7261b) `CredentialCrypto._load_or_create_key` was silently minting a new Fernet key whenever the file was missing. Failure mode the audit flagged: partial restore where DB was restored but `/images/secrets/` was lost → working-looking system where every authenticated download fails AUTH_ERROR until operator re-uploads every credential by hand. Two opt-ins now required for auto-create: 1. Explicit `bootstrap_ok=True` kwarg (tests, scripts), or 2. `CURATOR_BOOTSTRAP_NEW_KEY=1` env var (operator first-time-setup opt-in) Otherwise raises `MissingCredentialKey` so the app fails fast at startup. Loud log on key creation. Docstring corrected (was: "images/data root" / actual: `/images/secrets/credential_key.b64`). `tests/conftest.py` sets the env var at module load so existing test suites work unchanged. **Operator note:** if your running container has `/images/secrets/credential_key.b64` already present, no behavior change. If somehow the file is missing on the next deploy, the app refuses to start with a clear message telling the operator to restore from backup OR set the env var. ### Bundle D — ErrorType chip on FailingSourcesCard (f05aaa7) Alembic `0032` adds `Source.error_type` (varchar(32), indexed). `_update_source_health` stamps it on `status='error'` and clears on `'ok'`. `SourceRecord.to_dict` exposes it. `FailingSourcesCard.vue` renders a colored chip next to the consecutive-failures count, with a tooltip explaining the suggested operator action for each ErrorType category. Color reflects triage intent: - **warning** (yellow) — operator action needed (`auth_error`) - **info** (blue) — backend-paced (`rate_limited` / `timeout` / `network_error` / `partial` / `tier_limited`) - **error** (red) — likely terminal without intervention (`not_found` / `access_denied` / `validation_failed` / `unsupported_url` / `http_error` / `unknown_error`) The audit flagged this: backend computed 13 ErrorType categories but only the free-text `last_error` reached the operator. Bulk-triage by class ("all `auth_error` → rotate cookies", "12 `rate_limited` → just wait") required opening Logs per row. ## Test Plan - [ ] Add a credentialed source on a non-`(discord,pixiv)` token platform if one ever lands — confirm `auth_type_for` routes the credential correctly - [ ] Merge two tags via the directory UI — confirm a `recompute_centroid` task fires (visible in System Activity) - [ ] Wait for the daily `backfill_thumbnails` Beat tick or trigger manually — confirm library missing-thumbnail rows are repaired - [ ] Create a new fandom tag from the modal `kind:fandom` flow — confirm FandomPicker shows it without a page reload - [ ] Stop a worker mid-download to force a recovery sweep on a fresh artist — confirm no IntegrityError poisons the request - [ ] Verify that `/images/secrets/credential_key.b64` exists in your deployed container before this rolls out (so the app boot doesn't trip the new safety check) - [ ] Trigger a failing source on each ErrorType class — confirm the chip color matches the triage intent ## Pending in Group 5 (not in this PR) - **G5.1 — Centroid version two-sources-of-truth** (env `SIGLIP_VERSION` vs `MLSettings.embedder_model_version`): needs design decision before fixing. - **G5.4 — Modal cross-route navigation leaks**: needs UX behavior choice (RouterLink in modal closes vs preserves? `?image=N` strips on close?). - Duplicate tag-merge endpoint (`/api/tags` vs `/api/admin/tags`) — has 1 FE caller + 3 tests on the admin path; deserves its own consolidation change. ## Audit reference This closes Bundles A/B/C/D of Group 5 (plan task `Scribe #551`). After merge, G5.1 + G5.4 are the remaining open findings. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

bvandeusen added 6 commits 2026-06-02 18:07:20 -04:00

fix(audit-g5a): small architectural cleanups bundle ... 98673d4dca

Five small G5 findings from the 2026-06-02 audit. Each is local and
follows an established FC pattern.

- download_service: replace hardcoded ('discord','pixiv') tuple with
  auth_type_for(platform) == 'token'. A 7th token-platform now picks
  up the right credential path without touching this site.

- /api/tags/<source_id>/merge enqueues recompute_centroid.delay after
  merge so the target's centroid reflects its new image set
  immediately. Daily list_drifted catches it within 24h, but eager
  recompute closes the suggestion-quality dip in the meantime.

- backfill_thumbnails added to beat_schedule (daily). The task
  docstring claimed periodic Beat but the entry was never registered,
  so the library got no self-healing thumbnail repair; only the
  manual admin-UI button fired it.

- modal.createAndAdd pushes a kind='fandom' tag into
  tagsStore.fandomCache so FandomPicker sees the new fandom on next
  open. Was: cache-gated load (length===0) skipped refetch, new
  fandom invisible until full page reload.

- cleanup cluster:
  - Drop .webp from cleanup_service.unlink — thumbnailer only writes
    .jpg/.png; the third tuple member was dead code.
  - Drop effective_date from /api/gallery/scroll response — no FE
    consumer reads it. Service still computes the attribute for
    timeline ordering; this just trims the JSON.
  - Rename store.recentMinute → store.recentRuns across the
    systemActivity store + three consumers (SystemActivitySummary,
    QueuesTable, SystemActivityTab). The data is the last 200 runs
    (not actually "last minute"), so the name lied.

NOT in this bundle: the duplicate tag-merge endpoint
(/api/tags vs /api/admin/tags) is harder — has 1 FE caller and 3 tests
on the admin variant; consolidation is its own change.

fix(audit-g5a): ruff isort — platforms after patreon_resolver 75c63e1511

fix(audit-g5b): race-poisoning in three find-or-create sites ... 8d75ade1d5

Audit 2026-06-02 flagged three SELECT-then-INSERT sites that lost the
race under concurrent writers / recovery-sweep replays, poisoning the
outer transaction with an unrecoverable IntegrityError and crashing
the calling task. Same shape as the banked 2026-05-26 ImageProvenance
incident (see reference_scalar_one_or_none_duplicates memory).

Mirrors the importer._get_or_create pattern in all three: savepoint
via begin_nested + IntegrityError rollback to that savepoint + re-
SELECT to grab the row the other worker committed first.

- importer._capture_attachment: PostAttachment.sha256 UNIQUE. attachments.store
  is sha-addressed so both workers race to write the same on-disk path
  (shutil.copy2 + rename is idempotent), so no extra cleanup needed.

- TagService.find_or_create: partial uniqueness index on
  (name, kind, COALESCE(fandom_id, -1)). The previous docstring
  claimed "INSERT ... ON CONFLICT" but the implementation was
  SELECT-then-INSERT with no recovery.

- ArtistService.find_or_create: already had IntegrityError handling
  but did session.rollback() (unwinds the WHOLE transaction); now
  uses begin_nested + sp.rollback() so the surrounding request's
  progress isn't lost.

fix(audit-g5c): refuse silent Fernet key regeneration on partial restore ... 4df98171ab

Audit 2026-06-02: `_load_or_create_key` silently minted a new Fernet
key whenever the key file was missing — no log, no warning. The
failure mode the audit flagged: a partial disaster restore where the
DB was restored but `/images/secrets/` was lost would produce a
working-looking system in which every authenticated download fails
AUTH_ERROR until the operator re-uploads every credential by hand.

Two opt-ins now needed for auto-creation:
  1. Explicit `bootstrap_ok=True` kwarg (tests, scripts), OR
  2. `CURATOR_BOOTSTRAP_NEW_KEY=1` env var (operator first-time setup)

Otherwise the constructor raises `MissingCredentialKey` so the app
fails fast at startup and the operator can restore the key file
from backup before encrypted_blob rows go undecryptable.

Also: docstring path was wrong (said "images/data root" but actual
location is `/images/secrets/credential_key.b64`) — corrected.

Tests updated to pass `bootstrap_ok=True` explicitly, and two new
tests cover the safety behavior (missing-key-raises, env-var-bootstraps).

fix(audit-g5d): surface ErrorType taxonomy on FailingSourcesCard ... f05aaa707b

Alembic 0032 adds Source.error_type (varchar(32), indexed).
_update_source_health stamps it alongside last_error on status='error'
and clears it on 'ok'. SourceRecord/to_dict exposes it.

FailingSourcesCard renders a colored chip next to the consecutive-
failures count, with a tooltip explaining the suggested operator
action. Color reflects intent:
  - warning (yellow) — operator action needed (auth_error)
  - info (blue)      — backend-paced (rate_limited / timeout /
                       network_error / partial / tier_limited)
  - error (red)      — likely terminal without intervention
                       (not_found / access_denied / validation_failed /
                        unsupported_url / http_error / unknown_error)

Audit 2026-06-02: the backend computed 13 ErrorType categories but
only the free-text last_error reached the operator. Bulk-triage by
class ("all auth_error → rotate cookies", "12 rate_limited → just
wait") required opening Logs per row.

fix(audit-g5c): set CURATOR_BOOTSTRAP_NEW_KEY=1 in conftest ... 9f7261b9c0

CredentialCrypto's safety check fires on create_app() instantiation
because the test environment has no pre-seeded Fernet key file. Set
the bootstrap env var before any test imports so the auto-create path
is allowed during tests.

bvandeusen merged commit 6ef0fed41f into main 2026-06-02 18:07:25 -04:00

bvandeusen referenced this pull request 2026-06-02 19:09:44 -04:00

audit-g5 final + ML threshold default + kebab menu fix #54

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

No items

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate-bot bvandeusen

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: bvandeusen/FabledCurator#53