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.
This commit is contained in:
Edouard Vanbelle
2026-08-26 09:07:45 +02:00
parent 671e6ac0e7
commit 4ae1531286
2 changed files with 101 additions and 13 deletions
+95 -9
View File
@@ -1151,8 +1151,72 @@ filesystem and a remote backend for hours). It sweeps the cold tail:
reference forever.
- Reports imported / skipped-orphan / skipped-jpeg / failed counts.
**Phase 3 (release N+1): delete** the fallback module and the sidecar
directories, gated on the job reporting an empty tail.
**Phase 3: the job deletes, not a release.** *(revised 2026-08-26 —
supersedes "delete in release N+1, gated on an empty tail")*
Sidecars are **local disk**. A release cannot know whether every
instance has drained, so gating deletion on "the tail is empty" asks an
operator to coordinate a fact nothing reports — and there is no way to
know when, or whether, they will trigger the jobs at all. Instead the
import unlinks each sidecar it has successfully imported, so **each
instance drains itself** and the directory becomes removable once
genuinely empty.
Three constraints on that:
- **Verify readback before unlinking.** Import → read the derived blob
back through the normal stack → *then* delete. A store that reported
success but landed unreadable would otherwise take the last copy with
it. Cheap next to the decode already performed, and it is the
difference between a migration and a data-loss bug.
- **Only after the read-order flip.** Deleting while the sidecar is
still read *first* sends reads to the derived tier as a side effect of
the migration — its first production traffic arriving by accident
rather than by decision.
- **Opt-in** (`?delete_imported=true`). A migration that deletes on its
default setting is surprising, and it is the same instinct as
no-silent-auto-repair: early runs import only, so an operator can
inspect before committing.
Register it as a **scheduled tick**, not a boot-time trigger: it is
idempotent and resumable, so periodic is safe, whereas walking a large
`.thumbnails/` during startup delays readiness for nothing.
The only remaining *release* is removing the fallback read path once the
directories are empty — by which point no data is at stake.
### Prerequisite: one persist function (found 2026-08-26)
**Four render paths write a sidecar; only one also writes the derived
row.** `store_derived_blob` has a single call site — in
`render_and_persist_all_webp` — while `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 merely being untidy: an
on-demand render (cache miss, a size never generated, an evicted
sidecar) keeps producing un-migrated state *after* the import runs, so
the tail never empties and the deletion gate never opens.
So before the imports can converge, all render paths must go through
**one** `persist_thumbnail` that writes the sidecar, the derived blob
and the moka entry together — the same single-source move as
`storage.copy_file_satellites`. What it writes then becomes a policy in
one place, so "stop writing sidecars" is later a one-line change rather
than four edits.
Interim setting is **dual-write**, for two reasons: it is what makes the
backlog finite, and it leaves reads untouched while the derived tier is
still unproven. Cost is one extra local `fs::write` per render,
negligible beside the decode. Note the sidecar *read* path must survive
until the directories are empty regardless, so stopping the write early
buys nothing.
Cost to be aware of: `ThumbnailService` holds no `DedupService` — it is
a per-call parameter (`dedup: Option<&DedupService>`) — so the
consolidation threads it through those paths, and
`generate_and_persist` takes a `thumb_path` where it will need the
`blob_hash` instead.
### The "just delete it" opt-out is no longer universally safe
@@ -1211,13 +1275,35 @@ hardcoded SQL). New sources bolt on independently.
table. Lands with step 5. File-keyed, never in
`content_derived_blobs`. Register it in `copy_file_satellites` and
declare its version semantics.
10. **`derived_import` job + the dual-read fallback** — see the
migration section. Phase 3 (deleting the fallback and the sidecar
dirs) is a separate later release, gated on an empty tail. **The
HTTP ETag moves to the derived hash here**, with the read-order
flip and not before — see "HTTP ETag" under *Read path and
caching* for why keying it earlier would make the ETag describe a
tier the response did not come from.
10. **Import jobs + the dual-read fallback** — see the migration
section. Revised ordering as of 2026-08-26:
a. **Consolidate onto one `persist_thumbnail`** (dual-write). The
blocker: today only one of four render paths writes the derived
row, so the import can never converge. See *Prerequisite: one
persist function*.
b. **`thumb_derived_import`** (shipped) and **`thumb_attached_import`**
(shipped) — two jobs, not one, because the keying differs and that
difference is the security boundary. A third, `transcode_import`,
is still needed: `ImageTranscodeService` **already exists** and
caches `.transcoded/{ext}/{file_id}.{ext}`, so those must be
**re-keyed** file→content on import (legitimate only because a
transcode is derivable). Its `.skip` markers — a cached negative
verdict with no bytes — remain an open question.
c. **Flip the read order**, derived first. The HTTP ETag's
source-keyed fallback becomes unreachable here; the attached and
derived halves already landed early, forced by the attachment
case (see *HTTP ETag*).
d. **Enable deletion** in the import jobs (opt-in, readback-verified).
e. **Remove the fallback read path** once the directory no longer
*exists* — not merely once it is empty. Two reasons. Empty is a
momentary property an on-demand render can undo, whereas absence
is one-way and observable, so the job removes the directory after
draining it and that absence is the proof. And it is far cheaper
to test: existence is a single `stat`, while emptiness costs an
`opendir`/`readdir`/`closedir` — which matters if the fallback
ever gates on it per read rather than once at boot. The only
remaining release, and no data is at stake by then.
11. **`DedupService` → `BlobHandler` rename** — decided, mechanical,
34 files. Standalone commit, `src/AGENTS.md` updated with it. Can
land at any point; last is easiest, since every earlier slice
@@ -9,10 +9,12 @@
//! and records the mapping, after which the derived tier can become
//! authoritative and the sidecar can be deleted.
//!
//! **Thumbnails only, and that is permanent.** The table also holds
//! `kind = 'transcode'`, but transcoding lands *after* this migration, so
//! transcodes are born into the table and never pass through a sidecar era.
//! This job will not grow a transcode arm.
//! **Thumbnails only.** The table also holds `kind = 'transcode'`, and those
//! need their own import — `ImageTranscodeService` already exists and caches
//! to `.transcoded/{ext}/{file_id}.{ext}`, a different tree with a different
//! key. Importing them means **re-keying** file→content, which is legitimate
//! only because a transcode is derivable from the source bytes. Separate job;
//! this one will not grow a transcode arm.
//!
//! ### Idempotent by construction
//!