Commit Graph

16 Commits

Author SHA1 Message Date
Edouard Vanbelle 620800ba32 docs(plan): add step 12 — document the two satellite tables
The tables exist and carry COMMENT ON text, but nothing explains the
pair together, and the relationship is the part that matters: they hold
the same kind of artifact under two different keys, and the keying
difference IS the security boundary. Content keying shares one
derivation across identical content, which is exactly what must not
happen for user-supplied bytes — one user's uploaded preview would be
served for every file whose content matches. A single table with a kind
column would not prevent that; the split does.

Today that reasoning lives only in scattered prose across this plan and
two job doc-comments, so someone adding a third artifact type has
nothing to read.

Scopes the page: column-by-column structure including the parts that
mislead (nullable blob_hash meaning a negative verdict, uploaded_by
NOT NULL with no FK), worked examples that make the rule checkable,
lifecycle and which consistency job covers which failure, and the
NULL-handling trap that has already caused two bugs — comparison
against NULL silently excludes negative rows, which is correct for
refcounts and wrong for dangling checks.

Lands in docs/architecture/ beside backend-storage.md, which documents
the blob layer underneath.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-30 22:42:34 +02:00
Edouard Vanbelle ba6fe49c71 docs(plan): mark step 7 done, with what landed beyond the original scope
The plan still said "Scoped 2026-08-27, not started" for work that is
complete and validated against S3 — the first thing a reviewer reads,
describing the PR as unwritten.

Records the two pieces that were not in the original scoping: the memory
cache moving to content keying (it was still file-keyed, so identical
content held two RAM entries), and the engine binding a run to the flags
it started with. Also that `.transcoded/` legitimately persists where
external mounts exist, since hash-less callers have no content identity
— absence is only expected elsewhere.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-30 22:24:11 +02:00
Edouard Vanbelle 03246305f6 feat(thumbnails): the sidecar fallback disables itself
Step 10e was written as a removal release: delete the fallback read
path once the directories are empty. That has the same flaw as gating
deletion on an empty tail, one level up — sidecars are local disk, so
no release can know that every instance has drained.

The only removal that can actually be written is "if the tier is gone,
return". `initialize` now probes the size directories once at boot;
when absent, every fallback read short-circuits on a relaxed atomic
load and touches no filesystem. The code stays, costs nothing, and can
be deleted whenever — or never.

Two things had to change for absence to be reachable at all:

* `initialize` no longer creates the directories. It create_dir_all-ed
  all three at every boot, so the import job removed them and the next
  restart put them back — the absence this gates on was unreachable by
  construction. Found on a sandbox where the job had drained the tier
  and a restart left three empty directories behind. Nothing has
  written a sidecar since step 10d2, so there was nothing to create
  them for.
* The probe tests the size directories, not the root. On macOS Finder
  leaves a .DS_Store in the root, which blocks remove_dir there
  permanently; gating on the root would keep the fallback alive on
  every developer machine for a reason unrelated to thumbnails. No size
  directory means no sidecar.

Every sidecar read and existence check now goes through `read_sidecar`
/ `sidecar_exists`, so the guard exists once rather than at each of the
twelve sites that built a path and read it — the build-then-read pair
was duplicated six times over.

The import job's root removal reports its outcome instead of discarding
it. It is the one result an operator is waiting for, and "directory not
empty" with no sidecars left is a failure worth naming.

Falls open: the flag starts true, so a service constructed without
`initialize` behaves as before. A drain completing mid-process leaves
it stale-true until restart, which costs the same failed opens as
today; it never goes false while sidecars remain.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-30 13:41:05 +02:00
Edouard Vanbelle f1f327a6c4 refactor(consistency): blobs_consistency reads only the database
`blobs_consistency` probed `blob_exists` once per row and, under
`?deep=true`, read and re-hashed every blob. `backend_consistency`
already reports the same `blob_missing_from_backend` from its
merge-join — so the probe was duplicated work that found strictly less
(a DB walk cannot see backend-only orphans by construction) at N round
-trips instead of one enumeration. Every scheduled sweep paid for it.

All three physical checks move to `backend_consistency`:

* `blob_missing_from_backend` was already there; the duplicate is gone.
* `blob_corrupted` / `blob_unreadable` hook the matched arm of the
  merge-join, which holds exactly the key pairs worth reading. Guarded
  by `in_range` so a pair past the horizon is not read twice, and
  `params.deep` is persisted on a fresh run and read back on resume so
  a paused deep scan does not silently continue shallow.

Deep mode belongs there because it is backend work end to end: the
only DB input is the hash. Keeping it in `blobs_consistency` forced
that tenant to carry a backend for one flag.

What remains is the half that needs no backend: `refcount_mismatch`
and its repair. The constructor drops from five parameters to two —
no backend, no storage_entries, no storage_path_fallback — and
`?storage=<name>` / `?deep=true` are now inert there, which the
job description says outright.

`affected_files` is needed by both tenants, so it moves to a shared
`blob_diagnostics` module rather than being copied.
`PROBED_STORAGE_PARAM` moves to `backend_consistency`: it was defined
in `blobs_consistency` and re-exported, which is backwards once the
DB-only tenant has no entry to scope. The create-grace window goes
with the probe — it existed to avoid flagging a blob whose bytes had
landed before its row, and the refcount comparison reads one
consistent snapshot.

Known cost: `backend_consistency` returns `backend_unenumerable` on
Azure and mid-migration, so on those configs missing bytes now go
unreported where the per-row probe caught them. That argues for the
Azure enumeration impl, not for keeping the probe.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-30 13:41:05 +02:00
Edouard Vanbelle 791e2da4f4 docs(plan): the sequence was missing "stop writing sidecars"
Step 10 went ...enable deletion, then remove the fallback "once the
directory no longer exists". That gate is unreachable as written: while
persist_rendered dual-writes and the PUT still writes ext-, any render
or upload recreates the tree seconds after the job removes it, so the
directory never stays absent and (e) can never fire.

Adds it as d2, between deletion and fallback removal, with the split
that only became visible while implementing 10d.

Rendered sidecars can stop immediately — the read flip has landed,
existing files are untouched so un-imported boxes keep their fallback,
and a failed derived store costs a re-render rather than data, since
that content is regenerable by definition.

Uploaded ones cannot, yet. upload_thumbnail_impl logs and still returns
201 when store_attached_blob fails, which is safe only because the ext-
sidecar catches it. Remove the sidecar while the store is best-effort
and a user's preview vanishes silently behind a success response — and
these are precisely the bytes with no server-side render path. So the
PUT must become fatal first.

Order recorded explicitly: make it fatal, then drop the sidecar.
Reversed, it trades a silent data-loss window for an empty directory.
2026-08-30 13:41:05 +02:00
Edouard Vanbelle 647929ed11 docs(plan): negative verdicts get a nullable blob_hash
Resolves the .skip-marker question. The table can say "here is the
artifact" but not "there is deliberately no artifact", and absence of a
row is ambiguous — it means both never-attempted and attempted-and-not-
worth-it, which destroys the only thing a negative cache holds.

Two live cases, not one. Transcodes drop a .skip marker when WebP is not
smaller. Thumbnails return empty Bytes for undecodable sources and ones
over MAX_DECODE_PIXELS — RAM-only today, so after moka evicts, a
60-megapixel upload has its full decode attempted again, forever. Same
gap, same table, so do both together.

Nullable rather than a sentinel hash: a sentinel stops blob_hash naming
a real blob and every future reader has to know the lie. Costs are one
`AND blob_hash IS NOT NULL` in ContentDerivedReferenceSource, the same
guard on the dangling-derived check, and dropping NOT NULL.

Permanent vs transient is the load-bearing split, and today's code
cannot tell them apart: generate_and_persist collapses every error into
empty Bytes, timeouts and semaphore closures included. Survivable while
the sentinel lives in moka, which evicts. Persist that same signal and a
thumbnail that timed out once under load is unrenderable forever. So the
renderer must return a typed outcome first, and only
permanently-unrenderable earns a row. The asymmetry sets the default —
a wrongly-cached transient is silent and permanent, a not-cached
permanent only costs repeated work — so unclassified errors are treated
as transient.

No TTL and no renderer-version term. Time is the wrong axis: the verdict
is deterministic in (content, encoder) and does not decay, so a TTL
re-attempts an OOMing decode on a schedule while still leaving staleness
for most of the window after a deploy. What removes the need for a
mechanism is that negative rows are disposable — they hold no data, so
a library upgrade invalidates them with a DELETE ... WHERE blob_hash IS
NULL in the same migration as the dependency bump. Recorded explicitly
so nobody later builds the expiry logic this replaces.
2026-08-30 13:41:05 +02:00
Edouard Vanbelle c04f0c6824 docs(plan): scope step 7, the transcode dual-write
Investigated but deliberately not started — the remaining pieces span a
service, its DI wiring and an unresolved design question, and a
half-wired service is worse than none.

ImageTranscodeService does not write the derived tier at all today, the
same gap persist_rendered closed for thumbnails, and it must close
before transcode_import can converge or the import chases a growing
cache.

Three pieces, and the first is smaller than it looks. get_transcoded
takes file_id while the table is content-keyed, but the hash is already
in scope one frame up — file_retrieval_service::try_transcode is called
where dto.content_hash is live — so it is a parameter to thread, not a
lookup to invent. Explicitly NOT by hashing original_content on the
fly, which would be a BLAKE3 over the whole file per request.

Second, the service has no BlobHandler field, so this touches the
constructor and DI; ThumbnailService hit the same ordering problem and
solved it with a per-call parameter, which is the cheaper precedent.

Third, the .skip markers stay unresolved: a negative verdict has no
bytes, so it does not fit a table whose row points at a blob. Either
leave them local and recompute per instance, or model a sentinel. It is
the only genuinely open design question left in step 10.
2026-08-30 13:41:05 +02:00
Edouard Vanbelle 4997eb4fb0 docs(plan): sequence transcode_import behind its two prerequisites
Answering "when is transcode_import planned": it is the remaining third
of 10(b), but it must not be next, and both reasons were learned on the
thumbnail side rather than predicted.

Format has to move into `variant` first. Transcodes are inherently
multi-format and `variant` is keyed on size alone — the same gap found
in 10c that keeps JPEG thumbnails on the sidecar. Importing before that
means migrating into a schema that cannot hold the data without
collisions across formats. One migration unblocks both, which is the
argument for doing it before either.

And step 7 must precede the import. ImageTranscodeService writes only
its file-keyed disk cache today, so the import would run against a cache
that is still growing and never reach an empty tail — exactly the trap
persist_rendered had to close for thumbnails, where one of four render
paths recorded a row and the tail could never empty.

Order: format-in-variant → step 7 → transcode_import.

Also records why the .skip markers are still open: a cached negative
verdict has no bytes, so it does not fit a table whose point is pointing
at a blob.
2026-08-30 13:41:04 +02:00
Edouard Vanbelle 260e6bb74f feat(thumbnails): read the derived tier ahead of the sidecar
Step 10c. The sidecar is local disk — invisible to other instances,
uncarried by a backend migration, uncovered by any consistency job.
Reading the derived tier first is what makes that state deletable. Not
the cost it appears to be: CachedBlobBackend gives the blob read a local
disk cache and moka absorbs the repeats above it.

Not a two-line swap, for two reasons.

A derived MISS must fall through to the sidecar; the old code
terminated the lookup with `?` because it was last. While the imports
drain, most content has a sidecar and no row — terminating there would
report "no thumbnail" for nearly all of it.

And the derived tier is WebP-only. store_derived_blob writes image/webp
and keys `variant` on the size alone, with no format term, so a JPEG
request matches the WebP row and would be served the wrong codec. The
old ordering hid this because the .jpg sidecar won first. So the lookup
is gated to WebP, JPEG clients stay on the sidecar — and the sidecar
cannot be deleted for them until `variant` encodes format. That is a new
prerequisite for step 10e, recorded in the plan rather than discovered
later.

Also corrects the plan: I had written that this flip removes the
derived-hash ETag hazard. It does not. A first render still creates the
row as a side effect of producing the body, whatever the read order, so
two consecutive reads still straddle its appearance. The real fix is
resolving the ETag after generation on the 200 path — a 304 only fires
when the client already holds a validator, which implies the row exists.
That is a handler restructure, not an ordering change.
2026-08-30 13:41:04 +02:00
Edouard Vanbelle 2775e6d567 docs: the two sidecar-only paths are production-unreachable
Closing out step 10(a). The remaining `None` call sites in
persist_rendered looked like an open gap; they are not reachable in
production. Both `get_thumbnail` and the path variant of
`generate_all_sizes_background` are called only from the `ThumbnailPort`
impl, and nothing holds a `dyn ThumbnailPort` — which the existing note
in get_cached_thumbnail already recorded and a grep confirms. Live
renders go through get_thumbnail_from_blob and
generate_all_sizes_background_from_blob, both of which carry a
DedupService and dual-write.

So threading a DedupService through them would be work with no runtime
effect. Recorded at each call site instead, with the condition that
matters: gaining a real caller means taking a DedupService first, or the
gap persist_rendered exists to close reopens — sidecar-only output the
import can never see, so the tail never empties and the deletion gate
never opens.

Marks 10(a) done in the plan with that caveat stated rather than
implied.
2026-08-30 13:41:04 +02:00
Edouard Vanbelle 18649eb7b9 fix(thumbnails): drop derived-hash ETag until the read-order flip
The ETag is computed BEFORE the body. On a cache miss no
content_derived_blobs row exists, so thumbnail_content_id returned the
source-keyed form — then rendering created that row, and the next
request resolved to the derived hash instead. The validator changed as a
side effect of producing the body, making every first render
immediately stale.

Latent until aaf08532: before the consolidation, the on-demand render
path never wrote a derived row, so the flip had nothing to trigger it.
Fixing one gap exposed the other.

Caught by thumbnail_etag_content_keyed.hurl — two consecutive GETs of an
unchanged file stopped revalidating to 304.

The plan already said derived-hash keying must land WITH the read-order
flip and not before; I brought it forward anyway when the attachment
case forced the attached half. This is the evidence for the constraint,
so the plan now records the attempt and why it failed rather than
leaving the note as untested caution.

The attached lookup stays — it has no such window, since an upload
writes its row synchronously before any read can observe it, and it
fixes a real collision: a copy inherits the source hash, so an original
and a copy carrying different uploaded previews would otherwise share
one validator while serving different bytes.

The flip removes the hazard for the derived half too: once that tier is
authoritative it is populated before it is consulted, so no row can
appear between two reads.
2026-08-30 13:41:04 +02:00
Edouard Vanbelle 4ae1531286 docs(plan): job-driven sidecar deletion, and the persist-consolidation blocker
Two revisions from working through step 10.

**Deletion moves into the import jobs, not a release.** Sidecars are
local disk, so a release cannot know whether every instance has drained
— gating on "an empty tail" asks an operator to coordinate a fact
nothing reports, and there is no telling when or whether they trigger
the jobs at all. Each job unlinking what it has imported makes every
instance drain itself. Constrained three ways: verify the derived blob
reads back before unlinking (a store that reported success but landed
unreadable would otherwise take the last copy), only after the
read-order flip (or the derived tier takes its first production traffic
by accident), and opt-in, since a migration that deletes by default is
surprising. Scheduled tick rather than boot trigger — idempotent and
resumable, so periodic is safe, while walking .thumbnails/ at startup
delays readiness for nothing.

**Found while checking the dual-write assumption: it does not hold.**
store_derived_blob has ONE call site; fs::write(&thumb_path, …) has
five. get_thumbnail, generate_and_persist and
generate_all_sizes_background all persist sidecar-only. That breaks the
migration's premise rather than being untidy — on-demand renders keep
producing un-migrated state after the import runs, so the tail never
empties and the deletion gate never opens. One persist_thumbnail owning
sidecar + derived + moka is therefore a prerequisite, and it makes "stop
writing sidecars" a later one-line change instead of four edits. Noted
that ThumbnailService holds no DedupService, so it must be threaded
through.

Also corrects a claim I put in thumb_derived_import's own docs:
transcoding is NOT a later step. ImageTranscodeService exists and caches
.transcoded/{ext}/{file_id}.{ext}, so a third import is needed and it
must re-key file→content — legitimate only because a transcode is
derivable. Its .skip markers remain an open question.
2026-08-30 13:41:04 +02:00
Edouard Vanbelle ec61ce77f8 docs(plan): ETag moves to the derived hash at the read-order flip
The ETag shipped in fe9c4f49 is keyed on (source_hash, size, format),
which is one term short: a thumbnail is a function of those PLUS the
renderer. Change the encoder or a quality setting and identical inputs
produce different bytes under an unchanged ETag — the same staleness
class the commit fixed, one level down. It bites when an already-cached
thumbnail is re-rendered after a renderer change.

Keying on the derived blob's own hash removes the term entirely: the
ETag IS the hash of the bytes, so any output change invalidates by
construction. It is self-consistent for free, because store_derived_blob
is ON CONFLICT DO NOTHING — a re-render never displaces the stored row,
so the ETag always equals what the derived tier will serve. No renderer
version constant to remember to bump.

Records why it cannot land yet. The derived tier is read LAST by design,
so an ETag naming the derived hash would describe a tier the response
probably did not come from; sidecar and derived agree at creation but
diverge if a sidecar is re-rendered while the derived row stays pinned
by DO NOTHING. An ETag that lies about the body is worse than one that
is merely coarse. Also the tier is WebP-only (variant is the size, with
no format term) and empty for anything predating this work until
derived_import backfills.

So it lands at step 10 with the flip, keeping today's form as the
fallback for ungenerated variants and formats the tier does not hold.
The LEFT JOIN already planned for the read path returns the derived
hash in the same query, so it costs no extra round-trip.
2026-08-30 13:41:04 +02:00
Edouard Vanbelle a7b7045ed2 docs(plan): record the blobs_consistency -> chunks_consistency rename
Follows from the blob/chunk taxonomy already in this section: the job
iterates storage.blobs, which post-CDC holds chunks, so it inherits
whatever that table ends up called.

Two rules attached, because a job name is not an internal identifier —
it appears in POST /api/admin/jobs/<name>/trigger, in
background_runs.job_name, and in whatever dashboards operators built:

  * Travel with the schema rename, never ahead of it. A job called
    chunks_consistency iterating a table still called storage.blobs is
    more confusing than today's mismatch.
  * Never recycle `blobs_consistency`. Under the corrected taxonomy the
    manifest job IS the blob-level job, so the freed name looks
    available — and a name that survives a release while changing
    meaning silently breaks admin URLs and orphans run history.
    manifests_consistency is unambiguous either way, so exactly one job
    gets renamed rather than two swapping.

Also records what is explicitly NOT renamed: the `.blob` on-disk suffix,
where correcting it to `.chunk` would mean renaming every file in every
deployment's blob store — a migration that can fail halfway, for
clarity no consumer benefits from since nothing parses the suffix. And
file.blob_hash, whose semantics are unchanged.

Docs only.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-30 13:41:04 +02:00
Edouard Vanbelle 8b8cec0ba1 docs(plan): revise derived-blobs design
Enrich OxiCloud to maximise the use of `dedup` Engine

2 cases will be covered:
- blobs issues from other blobs (thumbnail automatic generation from blob)
- by filename (ex: thumbnail uploaded from users)

A local cache will be added when blobs are remote (S3 or similar)
2026-08-23 23:19:11 +02:00
Edouard Vanbelle 7663f803d3 fix: fix services accessig directly to localstorage
Prevent services accessing directly to localstorage and prefer using an astraction layer
to expose full blob. The abstraction layer (dedup services) will cover backend storage
election (local, s3, ...), encryption, etc

This change permit audio_metadata_service, media_metadaa_service, face_indexing_service to handle
blobs without worring of the backend.

note: prefered way to handle blob is the streamed way. Some services may not have this possibility
2026-08-02 22:20:46 +02:00