`backend_consistency ?deep=true` re-reads every chunk and re-hashes it
to catch silent bit-rot, and records `blob_corrupted` (severity
`data_loss`) naming `backend.backend_type()`. It was reading through the
live backend — which for a remote backend includes `CachedBlobBackend`,
whose `get_blob_stream` returns the local cached file and never touches
the remote on a hit.
So the attribution was false in both directions: rot on S3 hidden by a
good cached copy, and rot in the cache reported against a healthy S3 —
the second sending an operator to the wrong layer entirely.
Surfaced by a real run: 2022 chunks, 321 ms shallow, 1.5 s deep. That is
0.74 ms per chunk for a full read plus BLAKE3, sequential, over S3 —
impossible, and explained by every chunk being cache-warm. A genuine
uncached sweep is tens of seconds.
Adds `BlobStorageBackend::uncached()`, defaulting to `None`.
`CachedBlobBackend` returns its inner; `Retry` and `Swappable` forward
so the unwrap reaches the cache through them. `Swappable` resolves via
`current()` rather than capturing a handle, because it sits OUTSIDE the
cache — a DI-time snapshot would keep pointing at pre-cutover storage
and audit the backend a migration just moved away from.
Only the cache is peeled. The cache stores plaintext and the content
hash is over plaintext, so unwrapping past the encryption decorator
would hand back ciphertext and fail every blob it checked.
**No change to normal reads.** `uncached()` is called in exactly one
place, and the unwrapped handle is used at exactly one call site
(`verify_bytes`). Enumeration, every other job, and every request path
still go through the cached stack.
`?storage=<entry>` was already correct — `build_entry_backend` has no
cache decorator — so this only fixes the live-backend path, which is the
one that was silently fast.
Also reports `verified` in the run extras, on every run including zero.
A deep run that verified nothing and one that verified everything were
otherwise indistinguishable in the outcome, which is what made a 1.5 s
"deep" sweep look plausible in the first place. Same lesson as
`orphans_covered` on the Azure fallback: a check that cannot report its
own coverage will eventually be believed when it should not be.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Step 7 of docs/plan/derived-blobs.md, write path first — the plan is
explicit that fixing it before the import means transcode_import only
has to handle history, not a moving target.
ImageTranscodeService now reads and writes storage.content_derived_blobs
under kind='transcode', keyed by the BLAKE3 of the SOURCE content. The
hash is threaded in from file_retrieval_service, which already holds it
as dto.content_hash; hashing here would be a BLAKE3 over the whole file
on every request. Callers without one (external mounts) keep the local
cache untouched, which is what the service did before this tier existed.
Negative verdicts become rows rather than zero-byte .skip files. A
transcode that came out larger is deterministic in the content, so it is
worth remembering; the row survives moka eviction, a restart, and the
deletion of .transcoded/, none of which the marker does. Only that
verdict is persisted — a timeout or a read error returns Err and is
recorded nowhere, because a momentary failure written here would mark a
perfectly transcodable image hopeless with nothing to retry it.
Representation is a NULL blob_hash, per the plan: a sentinel hash would
stop blob_hash naming a real Blob and every consumer would need to learn
the exception. A CHECK keeps blob_hash and content_type NULL together —
a type without bytes describes nothing, bytes without a type cannot be
served.
Two consumers had to be corrected for NULLs first, both of which would
have broken on the first negative row ever written:
* satellites_consistency reported them as derived_dangling_blob at
data_loss severity. SQL comparison against NULL is NULL, so EXISTS was
false and a row correctly pointing at nothing read as an artifact that
had gone missing.
* blob_reference_sources::list_referenced_blobs decodes blob_hash into
String, so the first NULL would have failed the decode and taken the
whole enumeration down. It would also have been wrong if it decoded —
a negative row holds no reference, which is why the counting forms
(WHERE blob_hash = <hash>) already exclude it for free.
lookup_derived returns a three-way answer because Option collapses the
two cases a caller deciding whether to spend a decode most needs apart:
never attempted, versus attempted and known not worth it.
DedupService is attached after construction via a OnceLock. DI builds
the transcode service ~240 lines before DedupService exists, and the
retrieval path that needs it is wired earlier still, so a constructor
argument would mean reordering more than this is worth.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The admin panel had no repair toggle wired to anything but a hardcoded
name list naming the two refcount tenants, so `thumb_derived_import` and
`thumb_attached_import` could not be run in repair mode from the UI at
all despite supporting it. And nothing in the job list said what any
given job does or whether clicking Run on production writes anything.
Three defaulted methods on `JobHandler` and `RecoverableJobHandler`:
fn description(&self) -> &'static str
fn mutates(&self) -> Mutates // Never | Always | OnRepairOnly
fn repair_description(&self) -> Option<&'static str>
`RecoverableAdapter` forwards them — the registry only holds
`dyn JobHandler`, so a tenant's metadata is invisible otherwise, and
falling back to the defaults would report every recoverable job as
read-only, including the ones that delete files.
Three values rather than a boolean because a job can be read-only by
default and destructive under `?repair=true`; a boolean answers wrongly
for one of its two modes, and `false` on something that unlinks files is
the dangerous direction to be wrong in. `repair_description` returning
`Option` collapses "does it repair" and "what does repair do" into one
method: presence gates the toggle, content is the confirmation text —
which the frontend cannot invent, since correcting a counter and
deleting sidecars are not the same warning.
`OnRepairOnly` with no `repair_description` is rejected at registration:
it claims to mutate only under a flag it does not support.
All 17 registered jobs declare all three. The panel now renders the
description under each name, badges read-only jobs, confirms before a
plain run of a mutating one, and offers the repair variant off the
backend flag instead of the name list.
Descriptions are English in the trait, next to the behaviour: one in
`locales/*.json` rots invisibly the moment a job changes, and a
translator cannot know what `manifests_consistency` reconciles. i18n can
layer on later keyed by job name with these as the fallback.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The filter was strip_suffix(".webp"), but persist_rendered writes
{hash}.{format.ext()} — so any client not advertising WebP leaves
{hash}.jpg on disk. Correct only while the derived tier was WebP-only;
once variant carried the format (20261022000000) a JPEG sidecar became
ordinary content, and leaving it unclaimed would keep .thumbnails/
permanently non-empty — the very signal step 10e gates on. The migration
could never finish.
Both codecs are now claimed and the format comes from the file's own
extension, so a .jpg imports AS JPEG. Deriving the variant and
content_type from it rather than hardcoding WebP is the point: a
mislabelled row would serve the wrong codec to whoever the read path
then matched it for.
ThumbnailFormat::ALL exists so the claim list and the write path cannot
drift — adding a format without teaching the import about it would
strand that codec silently.
The `ext-` rejection now carries real weight. Previously .jpg was
rejected wholesale, so the two jobs could not overlap by construction;
now they share an extension and only the prefix separates them. Both
directions stay under test.
Caught by the cross-job assertion, which counts every real sidecar being
claimed exactly once — the fixture gained a .jpg and the total moved 3
to 4, which is the test noticing rather than a test to update.
Step 10(a), the blocker. Four render paths each wrote the sidecar and
exactly one also recorded the content_derived_blobs row, so an on-demand
render — a cache miss, a size never generated, an evicted sidecar —
produced state the migration could never see. That breaks the
migration's premise rather than being untidy: thumb_derived_import would
never reach an empty tail, so the gate for deleting the sidecar would
never open.
Now every rendered thumbnail goes through persist_rendered, which owns
what persisting means. Raw `fs::write(&thumb_path, …)` drops from five
sites to two: the one inside persist_rendered, and
store_external_thumbnail's `ext-{file_id}.jpg`, which is file-keyed and
legitimately a different thing.
The path that matters most already had what it needed:
get_thumbnail_from_blob — the REST handler's fallthrough on a cache miss
— holds `dedup` and simply never used it for persistence. It now
dual-writes at no cost.
render_and_persist_all_webp had its own copy of the dual-write logic;
that copy is gone, so retiring the interim dual-write later is one edit
here rather than a hunt.
Two paths still pass `None` and remain sidecar-only: `get_thumbnail`
(renders from an on-disk original) and `generate_all_sizes_background`
(the path variant; the _from_blob sibling has dedup). Closing those
means threading a DedupService in from their callers. Left visible as an
explicit `None` at the call site rather than an absent write — the gap
is now something a reader trips over instead of something they have to
notice is missing.
Adds ThumbnailFormat::mime() beside ext(), since the derived row needs a
media type and an extension without a matching one is how a WebP ends up
labelled JPEG.
Precondition for the merge-join in backend_consistency (step 6 /
option A of docs/plan/derived-blobs.md), landed separately because it
is independently useful and carries the risk.
Two contract changes on BlobStorageBackend::list_blob_hashes:
1. Entries MUST be in ascending hash order. Every shipped backend
already did this — local sorts within each shard and walks 00..ff,
and since the shard IS the hash prefix that is globally sorted; S3
and Azure list lexicographically by key and blobs/<xx>/<hash> sorts
identically to <hash>. It was accidental, and a future backend
enumerating in any other order would have silently made the
merge-join emit bogus blob_missing_from_backend findings at
data_loss severity.
2. The cursor is the last hash returned, not an opaque backend token.
This is what lets a caller resume from a checkpoint it already
holds — the merge-join keeps one cursor for both the DB walk and
the backend walk instead of a compound one, which in turn means
blobs_consistency's existing cursor format survives and no paused
run is stranded.
Local already derived its position from a hash; it now emits the bare
hash instead of "<shard>/<hash>", and still accepts both legacy forms
so a run paused across this deploy resumes. The bare-shard form works
through the same path unchanged, since "3f" sorts before every 64-char
hash beginning "3f".
S3 moves from continuation_token to StartAfter, which supports this
natively. One non-obvious case handled: a page can contain only
non-canonical keys (.tmp spool files, .corrupt sidecars), which are
filtered into `unknowns`, leaving `blobs` empty — a naive
blobs.last() would return no cursor and silently end enumeration while
is_truncated said otherwise, making an audit job under-report. It now
falls back to the last key seen; StartAfter is a string comparison, so
a non-hash resume point is fine. "Cursor is a hash" constrains what
callers may synthesise, not what backends may return.
Azure is unaffected — it does not implement list_blob_hashes (TODO,
inherits the NotSupported default).
Adds the first test for enumeration at all: ordering across shards with
deliberately out-of-order inserts, complete paged traversal, and
resume from a caller-synthesised cursor.
NOT verified against real S3 — no bucket available here. The local path
is covered by the new test; the StartAfter change is reasoned from the
API contract and needs exercising against a real bucket before it is
relied on.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Step 5, read path — Option 2 of the two shapes discussed: the derived
blob is consulted LAST, after the sidecar, not first.
Read order is now
moka -> ext-{file_id}.jpg -> {blob_hash}.webp on disk -> derived blob
For every thumbnail already on disk the new branch is never reached, so
the database stays off the hot path and a fault in it cannot break a
working gallery. It answers only what disk cannot: a thumbnail rendered
by another instance, or a box whose sidecar was never populated. Legacy
content keeps serving from disk until `derived_import` migrates it.
That inverts the plan's stated order deliberately. Derived-blob-first is
right for the END state, because it is what lets the sidecar be deleted;
sidecar-first is right transitionally, because the risky reordering
should happen after the table has been seen serving real reads. The flip
belongs in the release that removes the sidecar, and the comment at the
branch says so.
The existing precedence is preserved and now documented: the file-keyed
client upload (ext-) is checked BEFORE the content-keyed server render.
That ordering is a security property, not a preference — content-keyed
artifacts are shared across every file with that content, so checking
the file-keyed one first is what keeps one user's uploaded preview from
ever being served for another user's identical file.
Shape notes:
* `find_derived_blob` lands on DedupPort/DedupService as the read
counterpart of `store_derived_blob`, so ThumbnailService needs no pool
field — and therefore ThumbnailService::new, DI and three tests are
untouched.
* It carries `content_type`, which is what will retire the byte-sniffing
in the handlers once reads are table-primary.
* The parameter is `Option<&DedupService>`, concrete rather than
`&dyn DedupPort`: DedupPort uses native `async fn` and so is not
dyn-compatible, and ThumbnailPort is never used as a trait object
(checked) — both handlers hold the concrete Arc. `None` means
sidecar-only, which is exactly today's behaviour and what the abstract
port impl passes.
fmt, clippy --all-features --all-targets, 35 unit tests clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Prerequisite 0 of docs/plan/derived-blobs.md. The zero-ref manifest
sweep read:
WHERE m.ref_count <= 0
OR NOT EXISTS (SELECT 1 FROM storage.files f
WHERE f.blob_hash = m.file_hash)
That OR hardcodes "storage.files is the only thing that can reference a
manifest". A thumbnail manifest held by storage.content_derived_blobs
has ref_count = 1, so the first clause is false — but no files row names
a thumbnail's Blob hash, so NOT EXISTS is true, the OR fires, and the
manifest is deleted, its chunks dereferenced and the bytes reaped on the
next sweep. Landing content_derived_blobs before this fix would destroy
the derived tier on the first GC run.
The second clause is not merely defensive: it is the ONLY reap path for
bulk deletes (user cascade, empty_trash), where the PG trigger touches
storage.blobs but never decrements the manifest and the per-file
cleanup_if_orphaned call is skipped. So the fix has to preserve that
role, not just add tables to the NOT EXISTS. It is now the union of
every registered manifest-level source.
Assembled once, not per sweep. An earlier cut of this change put a
format! inside the DELETE, which made the most dangerous statement in
the file unreadable, un-pasteable into psql, and injection-shaped even
though every input is &'static str. The statement is now built at
construction and stored on DedupService, so:
* the reap loop runs a fixed statement with no string work,
* the SQL string is stable, so prepared-statement cache keys are too,
* a golden test pins it byte-for-byte — a reviewer reads the SQL in
the test rather than mentally evaluating the registry,
* initialize() logs it at debug with the contributing source names,
recovering the "paste it into psql" property the literal had.
The registry is mandatory rather than Option. An empty registry makes
"nothing references it" vacuously true for every row, so the builder
panics instead of emitting a statement that would delete every manifest
in the database; DedupService::new always registers the two built-in
sources, so that panic is unreachable by construction. There is a test
for it.
Adds ref_exists_sql to the port, defaulting to (count) > 0 and
overridden by FilesReferenceSource with a real EXISTS. Without it the
reap predicate would have traded today's short-circuiting NOT EXISTS
for a COUNT(*) = 0 that scans every referrer — a regression precisely
on heavily-deduplicated blobs, which is what GC walks most.
fmt, clippy --all-features --all-targets, and the 11 affected unit
tests all clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Step 1 of docs/plan/derived-blobs.md. Makes "who references this blob
hash" an extension point instead of SQL hardcoded in two places
(dedup_gc's reap predicate and blobs_consistency's refcount recompute,
both naming storage.files and storage.chunk_manifests directly). Adding
a blob-owning table without teaching those two risks silent orphaning:
GC sees ref_count = 0 and reaps live content.
Behaviour is unchanged — this commit only introduces the port and the
two sources that reproduce today's SQL. Wiring follows.
Two levels, not one. add_reference bumps chunk_manifests.ref_count first
and only falls back to storage.blobs.ref_count, so a reference lands on
whichever counter its hash names and the two must be recomputed
separately. RefLevel is a parameter rather than a property of a source,
because storage.files legitimately contributes at both: a manifest-less
legacy row references a chunk, a CDC row references a Blob. The
NOT EXISTS guard on the chunk-level files term is load-bearing — for a
single-chunk file the whole-file hash equals its lone chunk's hash, so
without it the row is counted at both levels.
SQL fragments rather than a per-hash count. blobs_consistency recomputes
with one query per page, the expected count inlined as correlated
subqueries; asking each source for a count per hash would turn that into
sources x rows round-trips. So sources emit a fragment the registry sums
into the existing page query, and count_references exists only for the
on-demand path where the candidate set is already filtered to
ref_count = 0.
Fragments use their own aliases (cnt_f, cnt_m) rather than the sweeps'
outer-row aliases (b, m). A fragment reusing `m` would shadow the outer
alias in the manifest sweep and silently correlate against itself;
there is a test for it.
The SQL builders are free functions so the shape can be asserted without
constructing a pool — sqlx's connect_lazy still needs a Tokio context,
and the fragments are pure string assembly anyway.
9 unit tests. fmt, clippy --all-features --all-targets, build clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
link are checking that email matches, +email alias are normalize into email
if email is already used on another account, link is not possible
not usurpation risk as the IDP is choosen by the admin
this make OIDC compliant with the invariant binding (issuer and subject)
admin can now rename their provider without breaking
clarifing federation_kind: report the kind of federation wired not the allowed login method
hybryd login method are still allowed
Edouard Vanbelle