diff --git a/docs/ingest.md b/docs/ingest.md index af6c5e65..f1c61a6d 100644 --- a/docs/ingest.md +++ b/docs/ingest.md @@ -182,6 +182,65 @@ A missing ffmpeg is not a file's fault at all. `MediaToolUnavailable` is recorde `failed`, and it is re-raised, which is precisely why it sits outside the `MediaError` family. One broken machine is not five thousand broken files. +## A preview per asset, and it is allowed to fail + +Every item also gets a thumbnail, stored content-addressed beside its content and named by +`asset.thumbnail_hash`. The M5 gallery is the reason: drawing a grid of hundreds of tiles by +decoding full-resolution images at request time is the wrong shape, and the cost is naturally +amortized here, where the bytes are already open and already decoded once. + +**A thumbnail hash is a cache key, not an identity.** It is absent from every release manifest, +`ReleaseService.verify` never recomputes it, and two machines may legitimately hold different +preview bytes for one image — [media.md](media.md) has the determinism argument. Losing every +thumbnail blob loses only the time to render them again. + +Everything else follows from that sentence. + +**A preview that will not render is not an `IngestFailure`.** That error means "this file did not +become an asset, so fix the file"; here the asset exists, its bytes are stored and nothing was +lost. So the hash stays NULL and the run carries on with no entry in the report. Putting it there +would tell an operator that data was lost when it was not, and would bury real loss under it. The +NULL *is* the record, which is why nothing is logged: it is exactly what the backfill looks for. + +**Frames get previews too.** The frames of a clip are deliberately never re-probed — the port +guarantees each one, and re-decoding would put our own encoder's output into an operator's +report. That argument is about metadata a caller reads back as fact, and a preview is reported to +nobody, so it does not carry over. A gallery with tiles for stills and blanks for frames would be +the worse outcome. + +**One edge, one cache.** `DEFAULT_THUMBNAIL_MAX_EDGE` is pinned at the port and is not a +parameter on ingest or on the backfill. A per-call edge would fork the cache into variants +nothing can tell apart from a hash, and the column holds one pointer. + +**A deduplicated asset has its NULL filled, and a value is never replaced.** Origin fields record +the first sighting and are never rewritten; a preview is not provenance, so filling an empty one +from whoever first held the bytes is not a rewrite. That is what makes re-ingesting a source +enough to give assets written before the cache existed their previews. + +### The backfill + +`IngestService.backfill_thumbnails(project_id)` renders a preview for every asset in a project +that has none — the remedy for all three things a NULL can mean, and idempotent, so a second pass +over a healthy project examines nothing. + +It reads the **blob**, never `asset.uri`: that path may be gone, renamed, or on another machine, +while `blob_store.get(asset.content_hash)` is what the workspace actually holds. + +Three phases, and the rendering is in none of them — the same rule as a run. The first +transaction collects ids and hashes, the rendering happens outside any transaction, and the last +re-reads each asset before writing so an ingest that filled a preview meanwhile is not clobbered. + +It reports rather than raises, on `ReleaseService.verify`'s terms: someone repairing a damaged +workspace needs the list, and one asset nobody can render must not abandon the other five +thousand. `ThumbnailBackfill` keeps `missing` (the content blob is gone — damage a preview pass +cannot repair) apart from `unreadable` (the bytes are there and will not decode). The second +reuses `IngestFailure` because the `UNSUPPORTED`/`CORRUPT` split says exactly the right thing +about stored bytes; the first does not, because `IngestFailureKind` answers "what is wrong with +this file" and a blob that is not there is not a file. + +There is no progress to poll: a backfill has no `IngestJob` row. If that is ever wanted it is a +task of its own, not a flag on this one. + ## The target batch With no `batch_id`, the run creates a draft named `batch_name` or, failing that, after the @@ -194,8 +253,6 @@ order for a directory and frame order for a clip. ## What is deliberately not here yet -- **No thumbnails.** Generating one per asset at ingest and recording `asset.thumbnail_hash` is - #21, for the M5 gallery. - **No background execution.** A run is synchronous and in-process. The service API is shaped so that moving it behind a queue changes the caller's waiting, not its vocabulary — which is why a job is created `pending` and why progress is read off the row rather than off a callback. diff --git a/docs/media.md b/docs/media.md index 5e33df41..a0c2109c 100644 --- a/docs/media.md +++ b/docs/media.md @@ -208,6 +208,10 @@ a verification pass recomputing one — source-content hashes are reproducible, hashes are not, and conflating the two is how a "verified" release starts failing on a different machine. +`asset.thumbnail_hash` is where that cache key is recorded, and it holds itself to every word of +this paragraph: it is absent from `Manifest`, `ReleaseService.verify` does not touch it, and a +NULL there is an ordinary state rather than a fault. See [ingest.md](ingest.md). + ## Refusals Two errors under one `MediaError` base, so an ingest can catch the family once, record the @@ -331,14 +335,19 @@ Neither library needs an import-linter change. The contracts forbid *frameworks* kernel — FastAPI, Typer, MCP, uvicorn — not third-party libraries, and ffmpeg is reached through `subprocess` and is not an import at all. -## What is deliberately not here yet +## Everything on this page now has a caller + +Three things used to be listed here as not built yet, and none of them is. `Source` came off the +list with registration, which records a clip's original rate and the decomposition parameters +chosen for it, built on `VideoMetadata` exactly as anticipated — see [sources.md](sources.md). +The `Asset` fields came off it with [ingest](ingest.md): what a probe reported is now stored as +`asset.format`, and a frame's `index`/`timestamp` land on the asset as +`frame_index`/`frame_timestamp` beside the source it was cut from. -- **No thumbnail write.** `thumbnail()` hands back bytes. Storing them content-addressed and - recording an `asset.thumbnail_hash` is the thumbnail-cache task, for the M5 gallery. +The thumbnail write came off it last. `thumbnail()` still only hands back bytes — storing them is +not the port's job — but those bytes now go into the blob store during the ingest loop, on both +paths, and the hash lands on `asset.thumbnail_hash`. The cache-key-not-identity rule above is +what that column is built on, and `IngestService.backfill_thumbnails` is how an asset that +predates it, or one whose preview would not render, gets caught up. See [ingest.md](ingest.md). -Two things used to be on that list and no longer are. `Source` came off it with registration, -which records a clip's original rate and the decomposition parameters chosen for it, built on -`VideoMetadata` exactly as anticipated — see [sources.md](sources.md). The `Asset` fields came off -it with [ingest](ingest.md): what a probe reported is now stored as `asset.format`, and a frame's -`index`/`timestamp` land on the asset as `frame_index`/`frame_timestamp` beside the source it was -cut from. Both ports are called from exactly one place, and that is where. +Both ports are called from exactly one place, and that is where. diff --git a/docs/persistence.md b/docs/persistence.md index aecf20f0..b09a84bb 100644 --- a/docs/persistence.md +++ b/docs/persistence.md @@ -82,7 +82,7 @@ the operation and not the individual write. | --- | --- | --- | | Relations that get mutated element by element | Child table — `batch_asset`, `annotation_job_asset` | Membership and per-asset progress are edited one row at a time and queried from the asset side. `batch_asset.position` preserves order. | | Immutable nested values | JSON column — `annotation_schema.classes`, `annotation.geometry`, `annotation.attributes`, `release.split` | A schema version must rehydrate byte-identical, and nothing queries a single `LabelClass` in SQL. Child tables would only add ordering columns. | -| An immutable value too large for a row | The **blob store**, with the row keeping its hash — `release.manifest_hash` | A release manifest lists every asset and every label; megabytes of it in a column would have to be read just to list a dataset's releases. Content-addressed, so it is verifiable and two identical releases share one document. See [releases.md](releases.md). | +| An immutable value too large for a row | The **blob store**, with the row keeping its hash — `release.manifest_hash`, `asset.thumbnail_hash` | A release manifest lists every asset and every label; megabytes of it in a column would have to be read just to list a dataset's releases. Content-addressed, so it is verifiable and two identical releases share one document. See [releases.md](releases.md). A thumbnail is the same storage decision for a different reason: it is a *cache*, so the row keeps a pointer that may be NULL and losing the bytes costs only the time to render them again. | | Timestamps | TEXT holding ISO-8601 **with offset** | SQLite's `DATETIME` storage drops the timezone. Domain timestamps are timezone-aware UTC and a naive value is rejected at construction. | Foreign keys are declared `ON DELETE CASCADE` — and the store issues @@ -116,8 +116,9 @@ MIGRATIONS: list[Migration] = [ Migration(version=7, name="source_provenance", upgrade=...), Migration(version=8, name="ingest_pipeline", upgrade=...), Migration(version=9, name="ingest_job_progress", upgrade=...), + Migration(version=10, name="asset_thumbnail", upgrade=...), ] -FORMAT_VERSION: int = MIGRATIONS[-1].version # 9 +FORMAT_VERSION: int = MIGRATIONS[-1].version # 10 ``` `initialize()` reads the version stamped in `_visionset_meta` and runs whatever is @@ -221,12 +222,22 @@ which is exactly what `0` and `[]` say; NULL is what a run that named no batch m child table on the criteria above: a per-file report is an immutable value read whole, and nothing queries a single failed file in SQL. +Migration 010 is plainer still: one nullable column, `asset.thumbnail_hash`, pointing at a cached +preview in the blob store. No foreign key, so 008's "a column carrying a key cannot arrive by +`ALTER` at all" limit does not bite, and no data pre-check to make — the column is a *cache*, so +NULL is not a legacy value something has to tolerate but the ordinary state of an asset nobody +has rendered a preview for yet. `IngestService.backfill_thumbnails` reads exactly that state. + The fresh-versus-migrated test is only as strong as how far back `_downgrade_to_version_one` walks, so every migration added there needs its undo added too. Migrations 006 and 007 are the two places that undo cannot borrow its DDL from `_tables`, because `_tables` no longer describes the shape it is restoring. Migration 009 is the one place that needs no undo of its own: its columns live on `ingest_job`, which 008's undo rebuilds from scratch, so restoring the generation-1 shape removes them along with everything else. +Migration 010 gets no such ride and has its own `DROP COLUMN` line — `asset` is only ever +altered, for the reasons 008 gives, so nothing later rebuilds it. The compensation is that 010's +real `ALTER` runs on the way back up from generation 1, which is why it needs no generation twin +of `test_migration_nine_alters_a_table_migration_eight_rebuilt`. `format_version` here is the *database* generation. Validating the on-disk workspace layout around it — directories, the blob-store root, what makes a directory a workspace at all — diff --git a/src/visionset/kernel/adapters/_tables.py b/src/visionset/kernel/adapters/_tables.py index 685f8114..f379e503 100644 --- a/src/visionset/kernel/adapters/_tables.py +++ b/src/visionset/kernel/adapters/_tables.py @@ -224,11 +224,11 @@ class AssetRow(Base): uri: Mapped[str] = mapped_column(String, nullable=False) width: Mapped[int | None] = mapped_column(Integer, nullable=True) height: Mapped[int | None] = mapped_column(Integer, nullable=True) - # The four below arrive by ``ALTER TABLE`` in migration 8 and are therefore - # declared last, in the order that migration adds them. #21's - # ``thumbnail_hash`` goes after them, for the same reason. + # The five below arrive by ``ALTER TABLE`` and are therefore declared last, + # in the order the migrations add them: four in migration 8, then + # ``thumbnail_hash`` in migration 10. # - # All four are nullable, and that is not a shortcut. ``asset`` could not be + # All are nullable, and that is not a shortcut. ``asset`` could not be # rebuilt the way migrations 6 and 7 rebuilt their tables: four tables carry # ``ON DELETE CASCADE`` keys into it (``batch_asset``, ``annotation``, # ``dataset_member``, ``annotation_job_asset``), and under @@ -258,6 +258,18 @@ class AssetRow(Base): frame_index: Mapped[int | None] = mapped_column(Integer, nullable=True) #: Seconds into the clip. The locator that survives a different rate. frame_timestamp: Mapped[float | None] = mapped_column(Float, nullable=True) + #: A cached preview in the blob store, added by migration 10. + #: + #: Nullable for a reason the four above do not share: this one is a *cache*, + #: so NULL is the ordinary state rather than a legacy one. It means "no + #: preview yet" whether the row predates migration 10, holds bytes that will + #: not render, or simply has not been reached — and + #: ``IngestService.backfill_thumbnails`` reads it to find all three. + #: + #: Unindexed, and deliberately: the one query over it walks a project's + #: assets and filters in Python, which is the shape ``Repository.list`` + #: already has. No foreign key either — it names a blob, not a row. + thumbnail_hash: Mapped[str | None] = mapped_column(String, nullable=True) #: The same bytes are the same asset: the backstop under the ingest pipeline's diff --git a/src/visionset/kernel/adapters/migrations.py b/src/visionset/kernel/adapters/migrations.py index d6623ef3..262e8cd8 100644 --- a/src/visionset/kernel/adapters/migrations.py +++ b/src/visionset/kernel/adapters/migrations.py @@ -377,6 +377,40 @@ def _add_ingest_progress_and_report(connection: Connection) -> None: _add_column(connection, cast(Column[object], column)) +def _add_asset_thumbnail(connection: Connection) -> None: + """Give an asset somewhere to point at its cached preview. + + The plainest migration in the file, and every way it is plain is an + argument rather than an omission. One column, so there is no ordering + question between siblings. No foreign key — it names a blob, not a row — so + migration 8's limit, that a column carrying a key cannot arrive by ``ALTER`` + at all, does not bite. Nothing to refuse and nothing to rebuild: ``asset`` + has four ``ON DELETE CASCADE`` children, and under ``PRAGMA foreign_keys = + ON`` dropping it would take them silently. + + No data pre-check, and here that is easier to claim than it was for + migration 9: the column is a *cache*, so NULL is not a legacy value that + something has to tolerate but the ordinary state of an asset nobody has + rendered a preview for yet. ``IngestService.backfill_thumbnails`` reads + exactly that state and is the remedy for it. + + Idempotent the way 3 to 5 and 9 are — the inspector check inside + ``_add_column`` *is* the ``checkfirst``, because migration 1 is + ``create_all`` of current metadata. + + Unlike migration 9, this one **needs its own undo** in the tests' + ``_downgrade_to_version_one``. Migration 9's columns rode back on migration + 8's rebuild of ``ingest_job``; ``asset`` is only ever altered, so nothing + later removes this column on the way to generation 1. The flip side is that + the walk back to generation 1 does exercise this ``ALTER`` for real, which + is why there is no generation-9 schema twin of + ``test_migration_nine_alters_a_table_migration_eight_rebuilt``. + """ + # ``.c`` is typed as the generic column collection; the entry is a real + # ``Column``, which is what ``CreateColumn`` needs. + _add_column(connection, cast(Column[object], AssetRow.__table__.c.thumbnail_hash)) + + MIGRATIONS: list[Migration] = [ Migration(version=1, name="initial_schema", upgrade=_create_initial_schema), Migration( @@ -419,6 +453,11 @@ def _add_ingest_progress_and_report(connection: Connection) -> None: name="ingest_job_progress", upgrade=_add_ingest_progress_and_report, ), + Migration( + version=10, + name="asset_thumbnail", + upgrade=_add_asset_thumbnail, + ), ] FORMAT_VERSION: int = MIGRATIONS[-1].version diff --git a/src/visionset/kernel/domain/__init__.py b/src/visionset/kernel/domain/__init__.py index 43a37b35..a4addbd3 100644 --- a/src/visionset/kernel/domain/__init__.py +++ b/src/visionset/kernel/domain/__init__.py @@ -39,6 +39,7 @@ IngestJob, IngestResult, IngestState, + ThumbnailBackfill, ) from visionset.kernel.domain.media import ( ImageFormat, @@ -163,6 +164,7 @@ "SplitAssignment", "SplitRecipe", "TaskGroup", + "ThumbnailBackfill", "VideoFrame", "VideoMetadata", "VideoProvenance", diff --git a/src/visionset/kernel/domain/asset.py b/src/visionset/kernel/domain/asset.py index ec2c9490..cf44498f 100644 --- a/src/visionset/kernel/domain/asset.py +++ b/src/visionset/kernel/domain/asset.py @@ -5,7 +5,7 @@ from typing import Literal from uuid import UUID, uuid4 -from pydantic import BaseModel, Field, field_validator, model_validator +from pydantic import BaseModel, Field, ValidationInfo, field_validator, model_validator from visionset.kernel.domain.media import ImageFormat @@ -33,6 +33,9 @@ class Asset(BaseModel): clip. ``format`` and ``source_id`` are absent on a row written before the ingest pipeline existed, and no default could be honest: the store cannot invent a format nobody probed. + + ``thumbnail_hash`` is optional for a different reason again, and it is the + one field here that is **not** provenance — see its own note below. """ id: UUID = Field(default_factory=uuid4) @@ -50,12 +53,34 @@ class Asset(BaseModel): frame_index: int | None = Field(default=None, ge=0) #: Seconds into the clip — the locator that survives a re-decomposition. frame_timestamp: float | None = Field(default=None, ge=0) + #: A cached preview in the blob store, or NULL when none has been rendered. + #: + #: **A cache key, not an identity**, which is the whole reason this can sit + #: beside the provenance fields without being one. It never enters a release + #: manifest and ``ReleaseService.verify`` never recomputes it: two machines + #: may hold different thumbnail bytes for one image, because determinism is + #: promised within a Pillow build rather than across them. Losing every + #: thumbnail blob loses only the CPU time to render them again. + #: + #: NULL therefore has one meaning with three causes — an asset written + #: before the cache existed, one whose bytes would not render, or one a run + #: has not reached yet. ``IngestService.backfill_thumbnails`` is the remedy + #: for all three, and reads exactly this state. Declared last, after the + #: columns migration 8 added, because it arrives by ``ALTER TABLE`` too. + thumbnail_hash: str | None = None - @field_validator("content_hash") + @field_validator("content_hash", "thumbnail_hash") @classmethod - def _content_hash_is_sha256_hex(cls, value: str) -> str: - if not _SHA256_HEX.fullmatch(value): - raise ValueError("content_hash must be 64 lowercase hex chars (SHA-256)") + def _is_sha256_hex(cls, value: str | None, info: ValidationInfo) -> str | None: + """Both fields name a blob, so both are checked by the one rule. + + A second regex for the second field is how the two drift apart. ``None`` + passes through because ``thumbnail_hash`` is optional and + ``content_hash`` is not — pydantic has already refused a missing one by + the time a validator runs. + """ + if value is not None and not _SHA256_HEX.fullmatch(value): + raise ValueError(f"{info.field_name} must be 64 lowercase hex chars (SHA-256)") return value @model_validator(mode="after") diff --git a/src/visionset/kernel/domain/ingest.py b/src/visionset/kernel/domain/ingest.py index fb158822..3eca28d2 100644 --- a/src/visionset/kernel/domain/ingest.py +++ b/src/visionset/kernel/domain/ingest.py @@ -185,3 +185,45 @@ def deduplicated(self) -> int: def failed(self) -> int: """How many items could not be read at all.""" return len(self.failures) + + +class ThumbnailBackfill(BaseModel): + """What one pass of ``IngestService.backfill_thumbnails`` found and repaired. + + A report rather than a count or an exception, on ``ReleaseVerification``'s + terms: someone running a repair over a damaged workspace needs the list and + not the verdict, and one asset nobody can render must not abort the other + five thousand. + + ``missing`` and ``unreadable`` are different faults with different remedies + and are never merged. A content blob that is gone is workspace damage a + thumbnail pass cannot repair and must not hide; a blob that is present and + will not decode is an asset that will simply never have a preview. + + That ``unreadable`` reuses ``IngestFailure`` is deliberate — the + ``UNSUPPORTED``/``CORRUPT`` split says exactly the right thing about stored + bytes, and the remedy is identical. That ``missing`` does **not** is equally + deliberate: ``IngestFailureKind`` answers "what is wrong with this file", + and a blob that is not there is not a file. Nothing here is a + ``WorkspaceCorrupt``, because raising would abandon the repair of every + healthy asset over one bad row. + """ + + model_config = ConfigDict(frozen=True, extra="forbid") + + project_id: UUID + #: Assets that now have a preview they did not have before this pass. + filled: tuple[UUID, ...] = () + #: Assets whose content blob is gone from the store. + missing: tuple[UUID, ...] = () + #: Assets whose stored bytes are present and will not render. + unreadable: tuple[IngestFailure, ...] = () + + @property + def examined(self) -> int: + """How many assets this pass found without a preview. + + Derived rather than stored, so it cannot disagree with the three lists + it counts — the rule ``IngestResult.created`` already follows. + """ + return len(self.filled) + len(self.missing) + len(self.unreadable) diff --git a/src/visionset/kernel/services/ingest_service.py b/src/visionset/kernel/services/ingest_service.py index c4ada4b4..0108be79 100644 --- a/src/visionset/kernel/services/ingest_service.py +++ b/src/visionset/kernel/services/ingest_service.py @@ -53,8 +53,16 @@ not a file's fault at all; it fails the job outright and is re-raised, which is precisely why ``MediaToolUnavailable`` sits outside the ``MediaError`` family. -**What is still deliberately not here.** No ``thumbnail_hash``: #21. No -background execution: a run is synchronous and in-process, and the API is shaped +**A preview is a cache, so it fails softly.** Every item also gets a thumbnail, +stored content-addressed beside its content and named by ``Asset.thumbnail_hash`` +— the M5 gallery's reason for this task. One that will not render is *not* an +``IngestFailure``: the asset exists and nothing was lost, so the hash stays NULL +and :meth:`backfill_thumbnails` is the remedy. That method is the same +four-transaction discipline applied to a different job — read the ids, render +outside any transaction, write once at the end. + +**What is still deliberately not here.** No background execution: a run is +synchronous and in-process, and the API is shaped so that putting it behind a queue changes the caller's waiting rather than its vocabulary — which is why a job is created ``pending`` and moved to ``running`` by whoever picks it up, even though today that is the same call. @@ -64,6 +72,7 @@ from io import BytesIO from pathlib import Path +from typing import BinaryIO from uuid import UUID from visionset.kernel.domain import ( @@ -79,6 +88,7 @@ Project, Source, SourceKind, + ThumbnailBackfill, normalize_name, require_move, ) @@ -214,6 +224,89 @@ def resume(self, job_id: UUID) -> IngestResult: return self._run(job.id, source, name, job.batch_id) + # --- the thumbnail cache ----------------------------------------------- + + def backfill_thumbnails(self, project_id: UUID) -> ThumbnailBackfill: + """Render a preview for every asset in the project that has none. + + The remedy for the three things a NULL ``thumbnail_hash`` can mean: an + asset written before the cache existed, one whose preview a run could + not render, and one an import put there by another route. Idempotent + and re-runnable — a second pass over a healthy project examines nothing, + because there is nothing left without a preview. + + **It reads the blob, never ``asset.uri``.** That path is where the bytes + came from and may be gone, renamed, or on a machine this is not running + on; ``blob_store.get(asset.content_hash)`` is what the workspace + actually holds. + + **Three phases, and the rendering is in no transaction** — the module + docstring's rule, applied to a different job. Only ids and hashes cross + out of the first transaction, because ``Repository.update`` replaces the + whole row and a model captured before a slow phase would undo anything + written during it. The last phase re-reads each asset before writing, so + an ingest that filled a preview meanwhile is not clobbered by this pass. + + Unlike an ingest there is no progress to poll: a backfill has no + ``IngestJob`` row and nothing that could carry counters. If that is ever + wanted it is a task of its own, not a flag on this one. + + Args: + project_id: the project whose assets to repair. + + Returns: + What was filled, what has no bytes left to render, and what will not + render — three lists, because they have three different remedies. + + Raises: + ProjectNotFound: no such project in this workspace. + """ + with self._workspace.unit_of_work() as uow: + self._require_project(uow, project_id) + pending = [ + (asset.id, asset.content_hash, asset.uri) + for asset in uow.assets.list(project_id) + if asset.thumbnail_hash is None + ] + + rendered: dict[UUID, str] = {} + missing: list[UUID] = [] + unreadable: list[IngestFailure] = [] + for asset_id, content_hash, uri in pending: + try: + with self._workspace.blob_store.get(content_hash) as content: + thumbnail_hash = self._workspace.blob_store.put( + BytesIO(self._workspace.image_processor.thumbnail(content, name=uri)) + ) + except FileNotFoundError: + missing.append(asset_id) + except MediaError as exc: + unreadable.append(_failure(uri, exc)) + else: + rendered[asset_id] = thumbnail_hash + + filled: list[UUID] = [] + with self._workspace.unit_of_work() as uow: + for asset_id, thumbnail_hash in rendered.items(): + asset = uow.assets.get(asset_id) + if asset is None: + continue + if asset.thumbnail_hash is None: + uow.assets.update(asset.model_copy(update={"thumbnail_hash": thumbnail_hash})) + # Counted as filled either way, because ``filled`` states an + # outcome rather than a write: an ingest that rendered this one + # while the pass was decoding leaves it with a preview it did + # not have when the pass began, which is what the caller asked + # about. Dropping it would leave the asset in no list at all. + filled.append(asset_id) + + return ThumbnailBackfill( + project_id=project_id, + filled=tuple(filled), + missing=tuple(missing), + unreadable=tuple(unreadable), + ) + # --- the run, phase by phase ------------------------------------------- def _run(self, job_id: UUID, source: Source, name: str, batch_id: UUID | None) -> IngestResult: @@ -328,6 +421,13 @@ def _read_directory( metadata = self._workspace.image_processor.probe(handle, name=str(path)) handle.seek(0) content_hash = self._workspace.blob_store.put(handle) + # No rewind before this one, and the asymmetry is the port + # contract rather than an oversight: ``ImageProcessor`` + # promises to seek to 0 itself, which is exactly what lets + # one open handle serve a probe, a hash and a thumbnail in + # any order. ``BlobStore.put`` promises nothing, which is + # why the rewind above is still the caller's job. + thumbnail_hash = self._cache_thumbnail(handle, name=str(path)) except MediaError as exc: failures.append(_failure(str(path), exc)) else: @@ -340,6 +440,7 @@ def _read_directory( height=metadata.height, format=metadata.format, source_id=source.id, + thumbnail_hash=thumbnail_hash, ) ) # After every item, read or refused alike: ``processed`` counts what @@ -363,6 +464,13 @@ def _read_video(self, source: Source, job_id: UUID) -> tuple[list[Asset], list[I also mean putting our own encoder's output into an operator's per-file report — a failure nobody could act on. + They *are* thumbnailed, and that is not a contradiction of the paragraph + above. What must not be re-derived is anything an operator reads back as + a fact about their clip; a preview is a cache artifact reported to + nobody, and a gallery showing tiles for stills and blanks for frames + would be the worse outcome for the sake of a rule about metadata. It is + this path's only use of ``ImageProcessor``. + Damage arrives once and terminally: ffmpeg yields the frames it managed and *then* says the bytes ran out, so the refusal is caught around the loop and what was extracted is kept. The loop is left by falling out of @@ -385,18 +493,24 @@ def _read_video(self, source: Source, job_id: UUID) -> tuple[list[Asset], list[I self._record_progress(job_id, processed=0, total=None, failures=failures) try: for frame in frames: - content_hash = self._workspace.blob_store.put(BytesIO(frame.content)) + uri = f"{source.path}#frame={frame.index}" + # One buffer serves both. ``put`` leaves it at the end and + # ``thumbnail`` seeks back to 0 itself, which is the same port + # contract the directory path leans on. + content = BytesIO(frame.content) + content_hash = self._workspace.blob_store.put(content) candidates.append( Asset( project_id=source.project_id, content_hash=content_hash, - uri=f"{source.path}#frame={frame.index}", + uri=uri, width=provenance.metadata.width, height=provenance.metadata.height, format=FRAME_FORMAT, source_id=source.id, frame_index=frame.index, frame_timestamp=frame.timestamp, + thumbnail_hash=self._cache_thumbnail(content, name=uri), ) ) self._record_progress( @@ -407,6 +521,33 @@ def _read_video(self, source: Source, job_id: UUID) -> tuple[list[Asset], list[I self._record_progress(job_id, processed=len(candidates), total=None, failures=failures) return candidates, failures + def _cache_thumbnail(self, content: BinaryIO, *, name: str) -> str | None: + """Render a preview and store it, or hand back NULL and carry on. + + Best effort by design, and the ``try`` is *inside* the caller's rather + than around it. A preview that will not render is not an + ``IngestFailure``: that error means "this file did not become an asset, + so go and fix the file", and here the asset exists, its bytes are + stored and nothing was lost. Letting the refusal reach + ``_read_directory``'s ``except MediaError`` would report a perfectly + good file as unreadable *and* leave behind the orphan blob that probing + first exists to prevent — a bug that would look like the feature + working. + + The NULL is the record, which is why nothing is logged here. It is + exactly the state :meth:`backfill_thumbnails` queries for, so a failure + describes its own remedy and a later pass repairs it. + + ``max_edge`` is not a parameter, here or anywhere. The port pins one + size; a per-call edge would fork the cache into variants that nothing + can tell apart from a hash, and the column holds one pointer. + """ + try: + rendered = self._workspace.image_processor.thumbnail(content, name=name) + except MediaError: + return None + return self._workspace.blob_store.put(BytesIO(rendered)) + def _store(self, project_id: UUID, candidates: list[Asset]) -> tuple[list[Asset], list[UUID]]: """Write the rows, reusing whatever content the project already holds. @@ -418,6 +559,17 @@ def _store(self, project_id: UUID, candidates: list[Asset]) -> tuple[list[Asset] The map is updated as the run proceeds, so two identical files inside one directory become one asset rather than a pair the new unique index would refuse at commit. + + A deduplicated candidate is otherwise discarded whole — its origin is + the *second* sighting and is never written — with one exception, and the + exception is precise. ``thumbnail_hash`` is not provenance but a cache, + so filling a NULL from a candidate that has one is not a rewrite; it is + the cache being populated by whoever first held the bytes. That is what + makes re-ingesting a source enough to give assets written before the + cache existed their previews. A value already there is **never** + replaced: a second encode yields the same blob on this machine and a + different one on another, so the swap would cost a write and buy + nothing. """ assets: list[Asset] = [] created: list[UUID] = [] @@ -430,6 +582,11 @@ def _store(self, project_id: UUID, candidates: list[Asset]) -> tuple[list[Asset] stored = uow.assets.add(candidate) known[stored.content_hash] = stored created.append(stored.id) + elif stored.thumbnail_hash is None and candidate.thumbnail_hash is not None: + stored = uow.assets.update( + stored.model_copy(update={"thumbnail_hash": candidate.thumbnail_hash}) + ) + known[stored.content_hash] = stored if stored.id not in seen: seen.add(stored.id) assets.append(stored) diff --git a/tests/kernel/test_ingest_service.py b/tests/kernel/test_ingest_service.py index 03e6c911..edb5857b 100644 --- a/tests/kernel/test_ingest_service.py +++ b/tests/kernel/test_ingest_service.py @@ -37,7 +37,9 @@ InvalidName, InvalidTransition, MediaToolUnavailable, + ProjectNotFound, SourceNotFound, + UnsupportedMedia, ) from visionset.kernel.adapters import PillowImageProcessor from visionset.kernel.domain import ( @@ -59,7 +61,11 @@ VideoMetadata, VideoProvenance, ) -from visionset.kernel.ports import FRAME_FORMAT +from visionset.kernel.ports import ( + DEFAULT_THUMBNAIL_MAX_EDGE, + FRAME_FORMAT, + THUMBNAIL_FORMAT, +) from visionset.kernel.services import ( BatchService, IngestService, @@ -143,6 +149,26 @@ def thumbnail( return self._real.thumbnail(content, max_edge=max_edge, name=name) +class _ThumbnaillessProcessor: + """A decoder that reads every file and renders a preview for none of them. + + The refusal is a `MediaError`, which is the case that matters: a preview is + a cache, so it must degrade to a NULL rather than travel out as an + `IngestFailure` about a file that is perfectly good. + """ + + def __init__(self) -> None: + self._real = PillowImageProcessor() + + def probe(self, content: BinaryIO, *, name: str | None = None) -> ImageMetadata: + return self._real.probe(content, name=name) + + def thumbnail( + self, content: BinaryIO, *, max_edge: int = 256, name: str | None = None + ) -> bytes: + raise UnsupportedMedia("no preview today") + + def _planted_video_source(workspace: WorkspaceService, project: Project, tmp_path: Path) -> Source: """A video source written straight to the store, because registering probes. @@ -185,9 +211,32 @@ def __init__(self, tmp_path: Path, name: str = "ws") -> None: def clip(self, name: str = "clip.mp4", **kwargs: object) -> GeneratedVideo: return write_video(self.tmp_path / name, **kwargs) # type: ignore[arg-type] + def blob_hashes(self) -> set[str]: + """Every blob on disk, named by its hash — `put` is content-addressed.""" + return {path.name for path in (self.root / "blobs").rglob("*") if path.is_file()} + def blob_count(self) -> int: - """Blobs on disk. `put` is content-addressed, so this counts distinct bytes.""" - return len([path for path in (self.root / "blobs").rglob("*") if path.is_file()]) + """Blobs on disk, **cached previews included**: distinct bytes of any kind.""" + return len(self.blob_hashes()) + + def content_blob_count(self) -> int: + """The same, with the previews subtracted — ingested content only. + + Every deduplication claim in this file is about *content*: the same + image is stored once. Since #21 a preview is stored beside each asset, + so a bare `blob_count` would make "one image, one blob" read as two and + say nothing about the dedup it is there to prove. Every project in the + workspace is walked, because two of these tests ingest into two. + """ + previews: set[str] = set() + with self.workspace.unit_of_work() as uow: + for project in uow.projects.list(self.workspace.workspace_id): + previews |= { + asset.thumbnail_hash + for asset in uow.assets.list(project.id) + if asset.thumbnail_hash is not None + } + return len(self.blob_hashes() - previews) def assets(self) -> list[Asset]: with self.workspace.unit_of_work() as uow: @@ -456,7 +505,7 @@ def test_the_same_image_in_two_sources_is_one_blob_and_one_asset(tmp_path: Path) original = fixture.ingest.ingest(first.id) again = fixture.ingest.ingest(second.id) - assert fixture.blob_count() == 1 + assert fixture.content_blob_count() == 1 assert len(fixture.assets()) == 1 assert again.created == 0 assert again.deduplicated == 1 @@ -508,7 +557,7 @@ def test_two_identical_files_in_one_directory_become_one_asset(tmp_path: Path) - assert result.created == 1 assert len(result.assets) == 1 - assert fixture.blob_count() == 1 + assert fixture.content_blob_count() == 1 fixture.close() @@ -523,7 +572,7 @@ def test_re_ingesting_one_source_creates_nothing(tmp_path: Path) -> None: assert again.created == 0 assert again.deduplicated == 3 assert again.asset_ids == first.asset_ids - assert fixture.blob_count() == 3 + assert fixture.content_blob_count() == 3 fixture.close() @@ -540,7 +589,7 @@ def test_two_projects_ingesting_one_image_are_two_assets_over_one_blob(tmp_path: assert mine.asset_ids != theirs.asset_ids assert mine.assets[0].content_hash == theirs.assets[0].content_hash - assert fixture.blob_count() == 1 + assert fixture.content_blob_count() == 1 fixture.close() @@ -1139,6 +1188,338 @@ def test_resuming_an_unknown_job_is_refused(tmp_path: Path) -> None: fixture.close() +# --- a preview per asset -------------------------------------------------- + + +def _preview(fixture: Fixture, asset: Asset) -> ImageMetadata: + """Decode what was cached for `asset`, so the assertions can be about it.""" + assert asset.thumbnail_hash is not None + with fixture.workspace.blob_store.get(asset.thumbnail_hash) as cached: + return PillowImageProcessor().probe(cached) + + +def _blob_path(fixture: Fixture, content_hash: str) -> Path: + """Where `FilesystemBlobStore` keeps that hash, so a test can damage it.""" + return fixture.root / "blobs" / content_hash[:2] / content_hash[2:4] / content_hash + + +def _reread(fixture: Fixture, asset_id: UUID) -> Asset: + return next(asset for asset in fixture.assets() if asset.id == asset_id) + + +def test_every_ingested_still_gets_a_preview_in_the_blob_store(tmp_path: Path) -> None: + """The issue's first acceptance criterion: retrievable by hash.""" + fixture = Fixture(tmp_path) + write_images(fixture.stills, count=3) + source = fixture.sources.register_images(fixture.project.id, fixture.stills) + + result = fixture.ingest.ingest(source.id) + + assert all(asset.thumbnail_hash is not None for asset in result.assets) + assert all(fixture.workspace.blob_store.exists(a.thumbnail_hash or "") for a in result.assets) + # Three images and the three previews beside them, none of them shared. + assert fixture.blob_count() == 6 + assert fixture.content_blob_count() == 3 + fixture.close() + + +def test_a_preview_is_a_jpeg_whatever_the_source_was(tmp_path: Path) -> None: + """The port pins the encoding, so the cache holds one format and not four.""" + fixture = Fixture(tmp_path) + write_image(fixture.stills / "a.png", seed=1) + write_image(fixture.stills / "b.jpg", seed=2) + source = fixture.sources.register_images(fixture.project.id, fixture.stills) + + result = fixture.ingest.ingest(source.id) + + assert {asset.format for asset in result.assets} == {ImageFormat.PNG, ImageFormat.JPEG} + assert {_preview(fixture, asset).format for asset in result.assets} == {THUMBNAIL_FORMAT} + fixture.close() + + +def test_a_preview_is_bounded_by_the_port_s_pinned_edge(tmp_path: Path) -> None: + """Asserted against the constant, never a number copied out of it.""" + fixture = Fixture(tmp_path) + write_image(fixture.stills / "big.png", size=(1024, 512)) + source = fixture.sources.register_images(fixture.project.id, fixture.stills) + + asset = fixture.ingest.ingest(source.id).assets[0] + + preview = _preview(fixture, asset) + assert max(preview.width, preview.height) == DEFAULT_THUMBNAIL_MAX_EDGE + assert (asset.width, asset.height) == (1024, 512) + fixture.close() + + +def test_a_small_image_is_not_enlarged_into_its_preview(tmp_path: Path) -> None: + """`thumbnail` never upscales, so the cache is not bigger than the asset.""" + fixture = Fixture(tmp_path) + write_image(fixture.stills / "small.png") + source = fixture.sources.register_images(fixture.project.id, fixture.stills) + + asset = fixture.ingest.ingest(source.id).assets[0] + + preview = _preview(fixture, asset) + assert (preview.width, preview.height) == (asset.width, asset.height) + fixture.close() + + +def test_the_same_image_renders_the_same_preview_blob(tmp_path: Path) -> None: + """Repeatability, asserted as an equality — never against a hardcoded hash. + + Determinism is promised within one Pillow build and not across them, which + is the whole reason a thumbnail hash is a cache key rather than an identity. + Two projects are used because within one, dedup would make this trivially + true by never rendering the second. + """ + fixture = Fixture(tmp_path) + write_image(fixture.stills / "shared.png", seed=7) + other = fixture.projects.create("second-project") + here = fixture.sources.register_images(fixture.project.id, fixture.stills) + there = fixture.sources.register_images(other.id, fixture.stills) + + mine = fixture.ingest.ingest(here.id) + theirs = fixture.ingest.ingest(there.id) + + assert mine.assets[0].thumbnail_hash == theirs.assets[0].thumbnail_hash + # One image, one preview: two assets over two blobs, not four. + assert fixture.blob_count() == 2 + fixture.close() + + +def test_two_identical_files_share_one_preview_blob(tmp_path: Path) -> None: + fixture = Fixture(tmp_path) + write_image(fixture.stills / "a.png", seed=3) + write_image(fixture.stills / "b.png", seed=3) + source = fixture.sources.register_images(fixture.project.id, fixture.stills) + + fixture.ingest.ingest(source.id) + + assert fixture.blob_count() == 2 + fixture.close() + + +def test_a_frame_gets_a_preview_too(tmp_path: Path) -> None: + """The no-re-probe rule is about reported metadata, not about the cache.""" + fixture = Fixture(tmp_path) + clip = fixture.clip(fps=10, duration_seconds=1.0) + source = fixture.sources.register_video(fixture.project.id, clip.path, extraction_fps=1.0) + + result = fixture.ingest.ingest(source.id) + + assert result.assets + assert all(asset.thumbnail_hash is not None for asset in result.assets) + assert {_preview(fixture, asset).format for asset in result.assets} == {THUMBNAIL_FORMAT} + fixture.close() + + +def test_a_preview_that_will_not_render_leaves_a_null_and_no_failure(tmp_path: Path) -> None: + """A cache miss is not data loss, so it must not reach the per-file report.""" + root = tmp_path / "ws" + workspace = WorkspaceService.init(root, image_processor_factory=_ThumbnaillessProcessor) + ingest = IngestService(workspace) + project = ProjectService(workspace).create("p") + stills = tmp_path / "stills" + stills.mkdir() + write_images(stills, count=2) + source = SourceService(workspace).register_images(project.id, stills) + + result = ingest.ingest(source.id) + + assert len(result.assets) == 2 + assert result.failures == () + assert all(asset.thumbnail_hash is None for asset in result.assets) + # The two images, and nothing beside them. + assert len([p for p in (root / "blobs").rglob("*") if p.is_file()]) == 2 + workspace.close() + + +def test_a_refused_file_still_leaves_no_blob_of_either_kind(tmp_path: Path) -> None: + """Probing first is what keeps this true once a second `put` joined the loop.""" + fixture = Fixture(tmp_path) + write_unsupported_file(fixture.stills / "notes.txt") + source = fixture.sources.register_images(fixture.project.id, fixture.stills) + + result = fixture.ingest.ingest(source.id) + + assert result.failed == 1 + assert fixture.blob_count() == 0 + fixture.close() + + +def test_re_ingesting_fills_a_preview_an_earlier_asset_never_had(tmp_path: Path) -> None: + """A cache is filled by whoever first holds the bytes; provenance is not. + + The planted asset stands in for one written before this column existed. + """ + fixture = Fixture(tmp_path) + path = write_image(fixture.stills / "one.png", seed=5) + source = fixture.sources.register_images(fixture.project.id, fixture.stills) + with fixture.workspace.unit_of_work() as uow: + planted = uow.assets.add( + Asset( + project_id=fixture.project.id, + content_hash=fixture.workspace.blob_store.put(path.open("rb")), + uri="somewhere/else.png", + ) + ) + + result = fixture.ingest.ingest(source.id) + + assert result.created == 0 + assert result.assets[0].id == planted.id + assert result.assets[0].thumbnail_hash is not None + # Origin still records the first sighting; only the cache was filled. + assert result.assets[0].uri == "somewhere/else.png" + assert result.assets[0].source_id is None + fixture.close() + + +def test_re_ingesting_does_not_replace_a_preview_that_is_already_there(tmp_path: Path) -> None: + fixture = Fixture(tmp_path) + write_image(fixture.stills / "one.png", seed=5) + source = fixture.sources.register_images(fixture.project.id, fixture.stills) + first = fixture.ingest.ingest(source.id).assets[0] + + again = fixture.ingest.ingest(source.id).assets[0] + + assert again.thumbnail_hash == first.thumbnail_hash + fixture.close() + + +# --- the backfill --------------------------------------------------------- + + +def _plant_unrendered(fixture: Fixture, count: int) -> list[Asset]: + """`count` assets holding real bytes and no preview, as a pre-#21 row would.""" + planted: list[Asset] = [] + for path in write_images(fixture.stills, count=count, first_seed=11): + with fixture.workspace.unit_of_work() as uow, path.open("rb") as handle: + planted.append( + uow.assets.add( + Asset( + project_id=fixture.project.id, + content_hash=fixture.workspace.blob_store.put(handle), + uri=str(path), + format=ImageFormat.PNG, + ) + ) + ) + return planted + + +def test_the_backfill_renders_a_preview_for_every_asset_missing_one(tmp_path: Path) -> None: + """The issue's second acceptance criterion.""" + fixture = Fixture(tmp_path) + planted = _plant_unrendered(fixture, count=3) + + report = fixture.ingest.backfill_thumbnails(fixture.project.id) + + assert report.project_id == fixture.project.id + assert set(report.filled) == {asset.id for asset in planted} + assert report.examined == 3 + assert report.missing == () and report.unreadable == () + assert all(asset.thumbnail_hash is not None for asset in fixture.assets()) + fixture.close() + + +def test_a_second_backfill_pass_finds_nothing_left_to_do(tmp_path: Path) -> None: + """Idempotent, and cheap to re-run: there is no state to reset between passes.""" + fixture = Fixture(tmp_path) + _plant_unrendered(fixture, count=2) + fixture.ingest.backfill_thumbnails(fixture.project.id) + before = fixture.blob_count() + + report = fixture.ingest.backfill_thumbnails(fixture.project.id) + + assert report.examined == 0 + assert fixture.blob_count() == before + fixture.close() + + +def test_the_backfill_leaves_an_asset_that_already_has_a_preview_alone(tmp_path: Path) -> None: + fixture = Fixture(tmp_path) + write_images(fixture.stills, count=2) + source = fixture.sources.register_images(fixture.project.id, fixture.stills) + ingested = fixture.ingest.ingest(source.id) + + report = fixture.ingest.backfill_thumbnails(fixture.project.id) + + assert report.examined == 0 + assert {a.thumbnail_hash for a in fixture.assets()} == { + a.thumbnail_hash for a in ingested.assets + } + fixture.close() + + +def test_the_backfill_reads_the_blob_rather_than_the_file_it_came_from(tmp_path: Path) -> None: + """`uri` may name a path that is long gone; the workspace holds the bytes.""" + fixture = Fixture(tmp_path) + planted = _plant_unrendered(fixture, count=1) + for path in fixture.stills.iterdir(): + path.unlink() + + report = fixture.ingest.backfill_thumbnails(fixture.project.id) + + assert report.filled == (planted[0].id,) + fixture.close() + + +def test_an_asset_whose_blob_is_gone_is_reported_rather_than_raised(tmp_path: Path) -> None: + """Workspace damage a preview pass cannot repair, and must not hide. + + Not a `WorkspaceCorrupt`: raising would abandon the repair of every healthy + asset over one bad row, which is the argument `ReleaseVerification` already + makes for reporting. + """ + fixture = Fixture(tmp_path) + planted = _plant_unrendered(fixture, count=2) + _blob_path(fixture, planted[0].content_hash).unlink() + + report = fixture.ingest.backfill_thumbnails(fixture.project.id) + + assert report.missing == (planted[0].id,) + assert report.filled == (planted[1].id,) + assert report.examined == 2 + assert _reread(fixture, planted[0].id).thumbnail_hash is None + fixture.close() + + +def test_stored_bytes_that_will_not_render_are_reported_by_remedy(tmp_path: Path) -> None: + """`IngestFailure` earns its reuse here: the split says the right thing.""" + fixture = Fixture(tmp_path) + planted = _plant_unrendered(fixture, count=2) + _blob_path(fixture, planted[0].content_hash).write_bytes(b"not an image at all") + + report = fixture.ingest.backfill_thumbnails(fixture.project.id) + + assert report.filled == (planted[1].id,) + assert [failure.kind for failure in report.unreadable] == [IngestFailureKind.UNSUPPORTED] + assert report.unreadable[0].name == planted[0].uri + assert planted[0].uri not in report.unreadable[0].reason + fixture.close() + + +def test_backfilling_an_unknown_project_is_refused(tmp_path: Path) -> None: + """An empty report would read as "nothing to do" rather than "no such thing".""" + fixture = Fixture(tmp_path) + + with pytest.raises(ProjectNotFound): + fixture.ingest.backfill_thumbnails(uuid4()) + fixture.close() + + +def test_backfilling_a_project_in_another_workspace_is_refused(tmp_path: Path) -> None: + fixture = Fixture(tmp_path) + elsewhere = Fixture(tmp_path, name="other") + + with pytest.raises(ProjectNotFound): + fixture.ingest.backfill_thumbnails(elsewhere.project.id) + + elsewhere.close() + fixture.close() + + # --- scope ---------------------------------------------------------------- @@ -1256,3 +1637,14 @@ def test_an_asset_may_carry_a_source_and_no_frame_position() -> None: source_id: UUID = uuid4() assert _asset(source_id=source_id).source_id == source_id + + +def test_a_thumbnail_hash_is_held_to_the_same_rule_as_a_content_hash() -> None: + """One validator covers both, because both name a blob.""" + with pytest.raises(ValidationError, match="thumbnail_hash must be 64"): + _asset(thumbnail_hash="nope") + + +def test_an_asset_may_carry_no_thumbnail_hash_at_all() -> None: + """NULL is the ordinary state of a cache, not a violation to tolerate.""" + assert _asset().thumbnail_hash is None diff --git a/tests/kernel/test_metadata_store.py b/tests/kernel/test_metadata_store.py index 1c076850..c3443051 100644 --- a/tests/kernel/test_metadata_store.py +++ b/tests/kernel/test_metadata_store.py @@ -105,8 +105,17 @@ def _seed(uow: UnitOfWork) -> list[tuple[str, UUID]]: ), ) ) + # `thumbnail_hash` on one and not the other: the flat mapper has to carry a + # cached preview and an absent one through the same round trip. first = uow.assets.add( - Asset(project_id=project.id, content_hash="a" * 64, uri="file:///1.png", width=8, height=6) + Asset( + project_id=project.id, + content_hash="a" * 64, + uri="file:///1.png", + width=8, + height=6, + thumbnail_hash="c" * 64, + ) ) second = uow.assets.add( Asset(project_id=project.id, content_hash="b" * 64, uri="file:///2.png") diff --git a/tests/kernel/test_migrations.py b/tests/kernel/test_migrations.py index d6f81b16..d792abb3 100644 --- a/tests/kernel/test_migrations.py +++ b/tests/kernel/test_migrations.py @@ -76,9 +76,19 @@ def _downgrade_to_version_one(store: SqliteMetadataStore) -> None: ) ) connection.execute(text("CREATE INDEX ix_source_project_id ON source (project_id)")) - # Migration 8's own undo. The index goes before the columns it names, - # because SQLite refuses to drop a column an index still references. + # Migrations 10 and 8's undo, newest column first. The index goes before + # the columns it names, because SQLite refuses to drop a column an index + # still references. + # + # Migration 10 needs its own line where migration 9 needed none: + # ``asset`` is only ever altered — it has four cascading children and + # legitimate pre-pipeline rows — so nothing below rebuilds it and takes + # ``thumbnail_hash`` away for free. The compensation is that migration + # 10's real ``ALTER`` runs on the way back up from here, which is why it + # has no generation twin of + # ``test_migration_nine_alters_a_table_migration_eight_rebuilt``. connection.execute(text("drop index if exists uq_asset_project_content_hash")) + connection.execute(text("alter table asset drop column thumbnail_hash")) connection.execute(text("alter table asset drop column frame_timestamp")) connection.execute(text("alter table asset drop column frame_index")) connection.execute(text("alter table asset drop column source_id")) @@ -511,6 +521,74 @@ def test_migration_nine_keeps_the_runs_a_workspace_already_recorded(tmp_path: Pa store.close() +def _downgrade_to_generation_nine(store: SqliteMetadataStore) -> None: + """Take ``asset`` back to the shape migration 8's ``ALTER``s left it in. + + A ``DROP COLUMN`` rather than a hand-written ``CREATE TABLE``, for the + reason ``_downgrade_to_generation_eight`` gives: these tests compare + ``sqlite_master`` *text*, and SQLite rewrites the stored statement by + deleting the dropped column's definition and leaving every other character + alone, where a retyped baseline would differ in whitespace and fail for a + reason about this file rather than about the schema. + """ + with store.engine.begin() as connection: + connection.execute(text("alter table asset drop column thumbnail_hash")) + connection.execute(text("update _visionset_meta set format_version = 9")) + + +def test_migration_ten_gives_an_asset_a_place_for_its_preview(tmp_path: Path) -> None: + store = SqliteMetadataStore(tmp_path / "visionset.db") + store.initialize() + with store.engine.connect() as connection: + columns = {c["name"] for c in inspect(connection).get_columns("asset")} + assert "thumbnail_hash" in columns + store.close() + + +def test_migration_ten_leaves_an_asset_it_did_not_render_alone(tmp_path: Path) -> None: + """NULL is the ordinary state of a cache, not a legacy value to tolerate. + + Nothing is refused and nothing is dropped, and here that is easier to claim + than it was for migration 9: an asset written before this column has no + preview, which is precisely what NULL says, and + ``IngestService.backfill_thumbnails`` is the remedy that reads it. + + There is no schema twin of + ``test_migration_nine_alters_a_table_migration_eight_rebuilt`` because + migration 10 does not need one. That test exists because migration 8 + *rebuilds* ``ingest_job``, so a walk back to generation 1 re-creates + migration 9's columns from ``_tables`` and 9 never runs as an ``ALTER``. + ``asset`` is only ever altered, so + ``test_a_fresh_database_and_a_migrated_one_have_the_same_schema`` already + exercises this migration's real ``ALTER TABLE ... ADD COLUMN``. + """ + store = SqliteMetadataStore(tmp_path / "visionset.db") + store.initialize() + with store.engine.begin() as connection: + connection.execute(text("insert into workspace (id, name) values ('w', 'ws')")) + connection.execute( + text("insert into project (id, workspace_id, name) values ('p', 'w', 'proj')") + ) + _downgrade_to_generation_nine(store) + with store.engine.begin() as connection: + connection.execute( + text( + "insert into asset (id, project_id, modality, content_hash, uri) " + f"values ('a', 'p', 'image', '{'a' * 64}', '/in/one.png')" + ) + ) + + store.initialize() + + with store.engine.connect() as connection: + row = connection.execute( + text("select thumbnail_hash from asset where id = 'a'") + ).scalar_one() + assert row is None + assert store.format_version == FORMAT_VERSION + store.close() + + def test_a_fresh_database_and_a_migrated_one_have_the_same_schema(tmp_path: Path) -> None: """Migration 1 is ``create_all`` of *current* metadata, so the two paths differ.