bvandeusen/FabledCurator
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases 13 Wiki Activity

revert: remove the placement reconciler — it manufactured the problem it solved
CI / lint (push) Successful in 2s
Details
CI / extension-version (push) Successful in 3s
Details
Build images / sign-extension (push) Successful in 3s
Details
Build images / build-agent (push) Successful in 6s
Details
CI / frontend-build (push) Successful in 26s
Details
CI / backend-lint-and-test (push) Successful in 33s
Details
Build images / build-web (push) Successful in 59s
Details
Build images / smoke-web (push) Skipped
Details
Build images / build-ml (push) Successful in 1m55s
Details
Build images / promote (push) Skipped
Details
CI / integration (push) Successful in 2m22s
Details

Browse Source 
Milestone #421 built a sweep that compared each image's `artist_id` to the
name of the directory holding its file, and called every mismatch a misplaced
image. It reported 33,789 of 63,605 as wrongly filed. That number described
the comparison, not the library.

What it actually was:

  32,475  (97.1%)  one artist's own folder, spelled differently
                   — Telepurte/ vs telepurte/. Same artist, same art.
     657  ( 2.0%)  loose at the images root
     328  ( 1.0%)  in a folder named after a different artist

And the 1% did not mean what the tool assumed either. `ImageProvenance`
records the post and source every file was downloaded from — the
authoritative answer, which the tool never consulted. Querying it for all 328:

    144  provenance agrees with the record  (move would be right)
     87  provenance agrees with the FOLDER  (the record is wrong; move wrong)
     53  provenance names SEVERAL artists   (no single correct folder)
     41  no provenance at all
      3  agrees with neither

So the sweep would have misfiled or arbitrarily picked for ~41% of the only
set it was really needed for. The system already knew where each file came
from; the tool inferred it from a column and a directory name instead.

Operator, 2026-09-21: *"the current system consistently records where items
are and where they came from this is just complicating something works and
doesn't need fixing."* Correct on both counts.

Removed: the service, the tasks, the model and migration 0099's table, the
/api/cleanup/layout and /placement/* endpoints, the Maintenance card and its
store actions, and the tests. 0100 drops the table (rule #22 — no legacy).

KEPT deliberately, per the operator:
- `utils.paths.canonical_subdir` — new filesystem imports derive their
  directory from the artist's slug, matching what the downloader always did.
  Not part of this tool; removing it would be churn that fixes nothing.
- The 327 files run 1 moved (InsoUwu/ -> insouwu/). Same artist either way,
  and the gallery renders them correctly.
- Everything from #4223 (three-gate dedup, 256-bit pHash) and #4234 (backup
  credential exclusion). Those fixed problems that were actually reported.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LVjrnpQjRgHdvq95rASoiR
This commit is contained in:
bvandeusen
2026-09-21 18:17:13 -04:00
co-authored by Claude Opus 5
parent 2dd9b956d5
commit 11a01a9686
14 changed files with 111 additions and 1850 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/app/services/library_layout.py
-434
View File
@@ -1,434 +0,0 @@
"""Milestone #421: where an image file BELONGS, and which rows are not there.
The library is keyed on the Artist row's `slug` — one directory per artist.
It grew a second (and third, and fourth) home for many of them because
`Importer._copy_to_library` used to name the destination after the IMPORT
folder while the downloader wrote under the slug. That writer is fixed
(`utils.paths.canonical_subdir`, task #4244); this module is the other half —
finding the rows whose files are still in the old places, and saying where
each one goes.
## Preview and apply share these predicates, they do not re-derive them
`_misplaced_conditions` and `destination_for` are the whole decision. The
report (task #4245) and the move (task #4246) both spread them rather than
writing their own — the house shape for rule 93, snippet #3087. A preview
that computes its set differently from the apply is a preview that can lie,
and here the apply RENAMES the operator's art.
## What writes and what does not
`survey_layout` and `plan_placement` are read-only — they report and they
record a plan. `apply_run` and `revert_run` are the only functions here that
rename a file or rewrite a row, and each does both for one row at a time,
updating the row only after its rename lands.
"""
from __future__ import annotations
from dataclasses import dataclass, field
from datetime import UTC, datetime
from pathlib import Path
from sqlalchemy import func, select
from sqlalchemy.orm import Session
from ..models import Artist, ImageRecord, LibraryPlacementRun
from ..utils.paths import canonical_subdir
# Top-level directories under the images root that are STORES, not artists.
# A sweep that treats these as misplaced artwork would relocate the
# thumbnail cache, the attachment blobs, or the credential key.
# thumbs/ sha-addressed thumbnail cache (NOT path-keyed — see below)
# attachments/ sha-addressed non-media blobs
# cookies/, secrets/ credential material
# _backups/, _quarantine/ backup artifacts; files pulled out of the library
RESERVED_TOP_LEVEL = frozenset({
"thumbs", "attachments", "cookies", "secrets", "_backups", "_quarantine",
})
def canonical_dir(images_root: Path, slug: str) -> Path:
"""The one directory an artist's files belong under."""
return images_root / slug
def _misplaced_conditions(images_root: Path, artist_id: int, slug: str) -> list:
"""Rows of `artist_id` whose file is NOT under that artist's canonical
directory. Spread into both halves — never restated.
The prefix carries a trailing separator on purpose: without it, artist
`ara` would match every path under `arbuzbudesh/`, and the sweep would
report one artist's whole library as correctly placed while quietly
skipping another's.
`startswith` compiles to LIKE, where `_` and `%` are wildcards, and this
does not escape them. That is safe ONLY because `utils.slug.slugify`
reduces a slug to `[a-z0-9-]` — neither character can reach the pattern.
Widen that charset and this needs `autoescape=True`, or `poch4n_art`
starts matching `poch4nXart` too.
"""
prefix = f"{canonical_dir(images_root, slug)}/"
return [
ImageRecord.artist_id == artist_id,
ImageRecord.path.is_not(None),
~ImageRecord.path.startswith(prefix),
]
def destination_for(path: str, images_root: Path, slug: str) -> Path | None:
"""Where `path`'s file belongs, or None when this row must not be moved.
None means: the path is outside the images root, or its top-level segment
is a reserved store. Both are refusals rather than errors — a row pointing
somewhere unexpected is exactly what should NOT be relocated automatically.
## The one place this diverges from `canonical_subdir`
A file sitting at the images ROOT with a known artist moves under that
artist's directory here, where `canonical_subdir` would leave it alone.
The two answer different questions. At import time an empty subdir means
no artist was resolved, so there is nothing to canonicalise against. Here
the row already CARRIES an artist_id, so a file at the root is an anomaly
with a known correct home — which is the whole point of the sweep.
(The 660 unattributed files at the root have no artist_id at all and are
not reachable from these predicates; task #4247 decides those.)
"""
p = Path(path)
try:
rel_dir = p.parent.relative_to(images_root)
except ValueError:
return None
parts = rel_dir.parts
if parts and parts[0] in RESERVED_TOP_LEVEL:
return None
sub = canonical_subdir(str(rel_dir) if str(rel_dir) != "." else "", slug)
if not sub:
# At the root, with an artist — see the docstring above.
return canonical_dir(images_root, slug) / p.name
return images_root / sub / p.name
@dataclass
class ArtistLayout:
"""One artist's verdict."""
artist_id: int
name: str
slug: str
canonical_rows: int = 0
misplaced_rows: int = 0
# The non-canonical top-level directories this artist's files sit in —
# "Conto", "StickySpoodge", … This is what makes the report readable as
# the family list the disk survey found.
stray_dirs: list[str] = field(default_factory=list)
collisions: list[str] = field(default_factory=list)
missing_files: int = 0
unmovable: int = 0
@dataclass
class LayoutReport:
artists: list[ArtistLayout] = field(default_factory=list)
total_rows: int = 0
canonical_rows: int = 0
misplaced_rows: int = 0
collision_count: int = 0
missing_files: int = 0
unmovable: int = 0
unattributed_rows: int = 0
def as_dict(self) -> dict:
return {
"total_rows": self.total_rows,
"canonical_rows": self.canonical_rows,
"misplaced_rows": self.misplaced_rows,
"collision_count": self.collision_count,
"missing_files": self.missing_files,
"unmovable": self.unmovable,
"unattributed_rows": self.unattributed_rows,
"artists": [
{
"artist_id": a.artist_id,
"name": a.name,
"slug": a.slug,
"canonical_rows": a.canonical_rows,
"misplaced_rows": a.misplaced_rows,
"stray_dirs": a.stray_dirs,
"collisions": a.collisions,
"missing_files": a.missing_files,
"unmovable": a.unmovable,
}
for a in self.artists
if a.misplaced_rows or a.collisions
],
}
def survey_layout(
session: Session, images_root: Path, *, check_disk: bool = True,
) -> LayoutReport:
"""Read-only blast radius for the consolidation.
`check_disk` stats every destination to find rows that would collide with
a file already there, and sources that have already gone missing. It is
the honest number and it is what the apply will refuse on, but it costs
one stat per misplaced row over NFS — turn it off when you only want
counts.
"""
report = LayoutReport()
report.total_rows = session.execute(
select(func.count(ImageRecord.id))
).scalar_one()
report.unattributed_rows = session.execute(
select(func.count(ImageRecord.id)).where(ImageRecord.artist_id.is_(None))
).scalar_one()
artists = session.execute(
select(Artist).order_by(Artist.slug)
).scalars().all()
for artist in artists:
layout = ArtistLayout(
artist_id=artist.id, name=artist.name, slug=artist.slug,
)
conds = _misplaced_conditions(images_root, artist.id, artist.slug)
owned = session.execute(
select(func.count(ImageRecord.id))
.where(ImageRecord.artist_id == artist.id)
).scalar_one()
rows = session.execute(
select(ImageRecord.id, ImageRecord.path).where(*conds)
).all()
layout.misplaced_rows = len(rows)
layout.canonical_rows = owned - len(rows)
strays: set[str] = set()
destinations: dict[str, int] = {}
for row_id, path in rows:
dest = destination_for(path, images_root, artist.slug)
if dest is None:
layout.unmovable += 1
continue
try:
top = Path(path).parent.relative_to(images_root).parts
strays.add(top[0] if top else "<root>")
except ValueError:
strays.add("<outside>")
key = str(dest)
if key in destinations:
layout.collisions.append(key)
else:
destinations[key] = row_id
if check_disk:
if not Path(path).exists():
layout.missing_files += 1
elif dest.exists():
layout.collisions.append(key)
layout.stray_dirs = sorted(strays)
report.artists.append(layout)
report.canonical_rows += layout.canonical_rows
report.misplaced_rows += layout.misplaced_rows
report.collision_count += len(layout.collisions)
report.missing_files += layout.missing_files
report.unmovable += layout.unmovable
return report
# --- the reconciler: plan -> apply -> revert (#4246) -------------------------
#
# The three verbs share one materialised plan rather than each deriving its
# own set. `plan_placement` writes `LibraryPlacementRun.moves`; `apply_run`
# executes THAT list; `revert_run` walks it backwards. A preview that can
# disagree with its apply is the failure this shape exists to prevent, and
# here the apply renames the operator's art.
#
# Every step fails CLOSED. The world moves between planning and applying —
# a download lands, a supersede rewrites a path, a file is deleted — so the
# apply re-checks each row against what the plan recorded and declines the
# ones that moved on, instead of trusting a plan that may be minutes old.
def plan_placement(
session: Session, images_root: Path, *, artist_id: int | None = None,
) -> LibraryPlacementRun:
"""Build (and persist) the move plan. Touches no files.
`artist_id` scopes the run to one artist, which is how this is meant to be
used: do one, look at the gallery, then continue or revert. None plans the
whole library.
"""
stmt = select(Artist).order_by(Artist.slug)
if artist_id is not None:
stmt = stmt.where(Artist.id == artist_id)
artists = session.execute(stmt).scalars().all()
candidates: list[dict] = []
wanted: dict[str, int] = {}
for artist in artists:
rows = session.execute(
select(ImageRecord.id, ImageRecord.path)
.where(*_misplaced_conditions(images_root, artist.id, artist.slug))
).all()
for row_id, path in rows:
dest = destination_for(path, images_root, artist.slug)
if dest is None or dest.exists():
continue
key = str(dest)
candidates.append({"image_id": row_id, "from": path, "to": key})
wanted[key] = wanted.get(key, 0) + 1
# Two rows wanting one destination: plan NEITHER. Which of them "wins" is
# not this sweep's call, and planning one of them would silently pick a
# winner by iteration order. Counting first and filtering after is what
# makes that true — claiming as we go would quietly keep whichever came
# first.
moves = [m for m in candidates if wanted[m["to"]] == 1]
run = LibraryPlacementRun(
status="ready", artist_id=artist_id, moves=moves,
planned_count=len(moves),
)
session.add(run)
session.flush()
return run
def _move_one(src: Path, dest: Path) -> str | None:
"""Rename `src` to `dest`. Returns a refusal reason, or None on success.
A rename within one filesystem, so no copy and no free space needed. The
destination check is not a race-free guarantee — nothing here is — but it
turns the common case of "something already landed there" into a refusal
instead of a silent overwrite.
"""
if not src.exists():
return "source missing"
if dest.exists():
return "destination occupied"
try:
dest.parent.mkdir(parents=True, exist_ok=True)
src.rename(dest)
except OSError as exc:
return f"rename failed: {exc}"
return None
def apply_run(
session: Session, run: LibraryPlacementRun, *, chunk: int = 0,
) -> LibraryPlacementRun:
"""Execute a `ready` run's stored plan: file and row together, per row.
The row is updated ONLY after its rename lands, so a refused or failed
move can never leave `ImageRecord.path` pointing at a file that is not
there. Refusals are recorded and the run continues — one row that moved
on since planning is not a reason to abandon the other 33,788.
`chunk` commits progress every N moves. Set it for any real run: the
ledger is the ONLY record of where a file came from, so a worker that
dies at row 30,000 of 33,789 must not take the undo information for the
first 29,999 with it. Left at 0 (tests, small runs) everything persists
in one go at the end.
Re-running a partially-applied plan is safe rather than clever: the rows
already moved no longer match their `from`, so they refuse as "row moved
since planning" instead of being moved twice.
"""
if run.status != "ready":
raise ValueError(f"run {run.id} is {run.status}, not ready")
refusals: list[dict] = []
moved: list[dict] = []
def _persist() -> None:
# Reassign rather than mutate: SQLAlchemy does not track in-place
# changes to a JSONB list, so an .append() alone would never reach
# the database and the ledger would silently stay empty.
run.moves = list(moved)
run.refusals = list(refusals)
run.moved_count = len(moved)
run.refused_count = len(refusals)
session.commit()
# Snapshot the plan before iterating: `_persist` reassigns `run.moves`,
# and iterating the attribute while rewriting it would walk a list that
# changes underneath the loop.
plan = list(run.moves)
for done, move in enumerate(plan, start=1):
record = session.get(ImageRecord, move["image_id"])
if record is None:
refusals.append({"image_id": move["image_id"], "reason": "row gone"})
continue
if record.path != move["from"]:
# Something rewrote this row since the plan was built — a
# supersede, or an earlier run. The plan is stale for it.
refusals.append({
"image_id": move["image_id"], "reason": "row moved since planning",
})
continue
reason = _move_one(Path(move["from"]), Path(move["to"]))
if reason is not None:
refusals.append({"image_id": move["image_id"], "reason": reason})
continue
record.path = move["to"]
moved.append(move)
if chunk and done % chunk == 0:
_persist()
run.moves = moved
run.refusals = refusals
run.moved_count = len(moved)
run.refused_count = len(refusals)
run.status = "applied"
run.finished_at = datetime.now(UTC)
session.flush()
return run
def revert_run(
session: Session, run: LibraryPlacementRun,
) -> LibraryPlacementRun:
"""Put an applied run's files back where they came from.
This is why `from` is retained. It is the answer to "do one artist, look
at it, and undo if it reads wrong" — which is a cheaper way to settle
whether artist_id or the folder held the truth (#4257) than arguing it
from a sample.
Refuses the same way the apply does: a file someone has since moved or
replaced stays where it is, and its row is left alone.
A revert interrupted half way is resumable by re-running it: the rows
already put back no longer sit at `to`, so they refuse rather than move
twice. Unlike the apply this needs no chunked persistence — it consumes
the ledger rather than producing it, so a crash costs progress, not
information.
"""
if run.status != "applied":
raise ValueError(f"run {run.id} is {run.status}, not applied")
refusals: list[dict] = []
reverted = 0
for move in run.moves:
record = session.get(ImageRecord, move["image_id"])
if record is None or record.path != move["to"]:
refusals.append({
"image_id": move["image_id"], "reason": "row changed since apply",
})
continue
reason = _move_one(Path(move["to"]), Path(move["from"]))
if reason is not None:
refusals.append({"image_id": move["image_id"], "reason": reason})
continue
record.path = move["from"]
reverted += 1
run.refusals = refusals
run.refused_count = len(refusals)
run.moved_count = run.moved_count - reverted
run.status = "reverted"
run.finished_at = datetime.now(UTC)
session.flush()
return run