ce4354f4971fcadde42d2dbc21dc995e9da4f07c
398 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Edouard Vanbelle
|
ce4354f497 |
fix(thumbnails): neither import job may tear down the shared directory
Found on a sandbox restore. `thumb_derived_import` ran first, imported and deleted its own hash-named sidecars, then found `remove_dir` refused because the `ext-*.jpg` previews were still there — those belong to `thumb_attached_import`. The rename fallback fired, moving the tree to `.thumbnails.migrated`; the attached job then looked in `.thumbnails/`, found nothing, and reported zeros. That stranded the user-uploaded previews, which are the one class of file here with no render path to rebuild them. The rename exists for files NEITHER job claims — a `.DS_Store` blocking removal forever — and it fired for the sibling's work in progress instead. Inverting the job order does not help: once the tree is renamed, both jobs look at `.thumbnails/` and find nothing, whatever order they run in. Teardown is now shared and refuses to act while anything remains that either job would claim. Both jobs call it, so whichever finishes last removes the tree in the same boot rather than leaving an empty directory until the next one. The rename survives for its original purpose, and now only fires when the remaining files are genuinely nobody's. Also drops the daily tick on both imports — they are on-demand now. The boot run in repair mode IS the migration: nothing has written a sidecar since step 10d2, so the tail cannot grow afterwards, and a tick could not finish the job anyway because ticks never pass `repair`. Once drained it was a `read_dir` returning nothing, every day, forever. UX: the "at boot" badge moves from beside the job name into the cadence column. It answers WHEN a job runs, which is what that column is for — next to the name it read as a property of the job, and the row could show "on-demand" beside a badge saying otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edouard Vanbelle
|
577ecb7cef |
feat(jobs): run the thumbnail migration at startup, by default
A migration nobody triggers never finishes. Scheduled ticks deliberately never pass `repair`, so a deployment whose operator never opens the admin panel re-imported the same sidecars forever and never drained the directory — and relying on operators to edit `.env` has the same failure mode one level up. `OXICLOUD_STARTUP_JOBS` dispatches named jobs once, in the background, after the scheduler is ready. Entries use the syntax operators already type at the trigger URL (`name?repair=true`), so the value is literally the request they would otherwise make by hand. It defaults to both migration jobs in repair mode, so an untouched deployment migrates and drains itself. That is a destructive default and a real exception to no-silent-auto-repair, so the guard it rests on had to get stronger: `verify_and_unlink` now compares CONTENT, not length. A blob of the right size and the wrong bytes used to pass — a key-mapping bug handing back another file's preview at the same length would have deleted the original and kept the impostor, and thumbnails cluster tightly enough in size for that to be a real coincidence. The readback streams from the backend with no cache in front, so it proves durability rather than that a write was acknowledged. Deletion of `.thumbnails/` is attempted first and only falls back to renaming it `.thumbnails.migrated` when `remove_dir` refuses because a non-sidecar file is inside (Finder's `.DS_Store`). Either way the directory stops existing, which lets the read-path probe go back to a single `stat` on the root instead of walking the size directories. Validation is fail-fast: an unknown job name or flag panics at boot. A silently dropped `?repare=true` would leave the job in discovery-only mode while the operator believed the tier was draining, surfacing months later as "the migration never finished" with nothing pointing at the config line. Interrupted runs resume. Boot recovery flips abandoned rows to Paused with their cursor, so `run_or_resume` continues rather than rescanning — a long migration completes across however many restarts it takes. That is a scoped exception to "we do not auto-resume": here somebody did ask, in configuration, and not having to ask again is the point. `StartupJob` holds a `JobRunArgs` rather than re-listing its four fields, so a fifth flag cannot be added to the scheduler and silently ignored in configuration. Jobs named here are ordinary registered jobs — visible in the panel, triggerable by hand, same runs and findings. Their rows now carry a `startup` object so an operator can see that a job deletes on every boot rather than only when someone clicks Run. Adds docs/config/thumbnail-migration.md: what runs on first boot, how to snapshot database and storage together beforehand, and how to verify afterwards with satellites_consistency plus backend_consistency ?deep=true. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
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> |
||
|
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> |
||
|
Edouard Vanbelle
|
1ea3826660 |
feat(jobs): jobs describe themselves — description, mutates, repair_description
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>
|
||
|
Edouard Vanbelle
|
b485db46fa |
feat(storage): audit every sidecar deletion, and reclaim orphaned uploads
Two changes to the import jobs' destructive path. thumb_attached_import now deletes orphaned sidecars under `repair`, matching the dead-source case on the derived side. An `ext-` file whose owner is gone is unimportable — the FK on file_id would reject the row — so leaving it means it is rediscovered every run, the tail never empties and step 10e's gate never opens. Safe despite these being the non-regenerable bytes: the preview is keyed to a file_id that no longer exists, so nothing can reference it again. Unrecoverable and unreachable are different things, and this is both. And every deletion is now audited. A one-way migration removing user-visible files should leave a trail that outlives the run history: findings are per-run and get purged, whereas target: "audit" is separable and retained. If a preview later turns out to be missing, this is the only record saying the migration removed it and when. `owner` carries the id the file belonged to — source_hash for content-keyed, file_id for uploaded — because that is where an investigation starts, and the raw logs cannot supply it: NEW BLOB names the hash of the STORED BYTES, a different value from the sidecar's own name, which is why grepping one against the other finds nothing. reason is a stable key: `imported` (replaced by a verified blob), `source_gone`, `orphaned`. The first lives inside verify_and_unlink so a verified deletion cannot be logged inconsistently; the other two are explicit, since those paths have nothing to verify against. |
||
|
Edouard Vanbelle
|
1a3d7d201a |
fix(storage): skip sidecars whose source is gone, before writing anything
Running the import on a real install produced a store-then-discard loop: NEW BLOB (CDC) immediately followed by MANIFEST DELETED, once per sidecar. store_derived_blob wrote the bytes, the source-exists guard refused the row, and `inserted == 0` released the reference again. The refusal is right — `.thumbnails/` outlives years of deleted files, and importing those would recreate exactly the orphan rows e4c78ae0 eliminated. The mistake was deciding it AFTER the write. Now checked before the read and the store, via blob_exists (manifest first, blob as fallback). Two costs it removes: a blob write plus a manifest delete per dead sidecar on EVERY run, and a tail that never empties — unimportable files are rediscovered forever, so the job never reports zero and step 10e's gate never opens. Reported as `sidecar_source_gone` so the scale is visible before anything is removed, and deleted under `repair`. That is the one unlink in this job needing no readback: there is nothing to read back and nothing to regenerate from. Counted separately in the completion log, because "skipped, source gone" and "already present" mean different things to an operator deciding whether the migration has converged. Worth noting for anyone reading the raw logs: NEW BLOB names the hash of the STORED BYTES, while the sidecar filename is the SOURCE hash. They are different values, so grepping the log hash against .thumbnails finds nothing. The new finding carries both. |
||
|
Edouard Vanbelle
|
b3221e265d |
feat(consistency): satellites_consistency covers both tables, and the sweep covers every job
Extends the derived check to `file_attached_blobs` and renames it, since the two tables are one concept — the content-keyed and file-keyed halves of "things attached to a Blob" — and `storage.copy_file_satellites` already established the vocabulary. The attached half is the one that cannot be recovered. `attached_dangling_blob` is data_loss with `recoverable: false`: those bytes were user-supplied and have no server-side render path, so nothing can regenerate them. Its derived twin carries `recoverable: true`, because a derived artifact is a pure function of its source and re-rendering restores it. Same finding shape, materially different stakes, and the detail says which. No orphan-mapping check on the attached side, deliberately: `file_id` is ON DELETE CASCADE, so a row cannot outlive its file. The database enforces what the derived table cannot, since a content hash has no row to point a foreign key at — which is exactly why only that half could rot. One job walking two tables needs a phase in the cursor, or an attached checkpoint would be replayed against the derived table and silently re-scan or skip. Two things the sweep was missing, found while checking whether every consistency job is actually exercised: drives_consistency and folders_consistency were registered but never run by any test. Now included; the list is exhaustive by intent. An unknown job was a warning-and-skip. That protected feature-gated builds at the cost of something worse: this list said `derived_consistency` for one commit after the rename and would have dropped that coverage without a word, leaving the suite green over a check that no longer ran. It fails now. |
||
|
Edouard Vanbelle
|
4fef34b230 |
feat(consistency): derived_consistency — the last coverage-matrix gap
Finds derived mappings whose Blob is gone on either side. Nothing else can, and that is the point rather than an oversight: every other job reasons from a Blob outwards, so a row whose SOURCE was reaped breaks none of their invariants — valid reference, exactly correct refcount, bytes present on the backend. Every check agrees the system is healthy while the artifact is pinned forever. A leak that looks like correctness, which is why it took four suite runs to name. Two findings: derived_orphan_mapping (inconsistent) — source_hash has neither a manifest nor a blob row, so purge_derived_blobs can never fire for it. Storage that grows and never reclaims. derived_dangling_blob (data_loss) — blob_hash has no Blob behind it. The mapping promises an artifact that is gone, so a read finds the row and then fails. Existence means EITHER table on both sides, since source_hash and blob_hash each name a Blob: a manifest for CDC content, a bare blob row for legacy whole-file content. Checking one would report every legacy blob as missing. Paged on the full primary key with a row-value comparison rather than source_hash alone — a source has several variants, so a page boundary can fall inside one and advancing by source would skip the rest. Both existence probes fold into the page query, so a page is one round-trip rather than 2xN. Cursor round-trip is tested, including that a malformed one fails loudly: silently restarting would make a paged audit under-report, which is the worst failure available to a job whose purpose is finding what is missing. e4c78ae0 stops new orphans at the write side; this finds the ones already on disk, which that fix cannot reach. Added to the end-of-suite sweep so it runs against real state every time. |
||
|
Edouard Vanbelle
|
de0f625d4c |
fix(dedup): refuse a derived mapping whose source is already gone
Permanent blob leak, three rows per image. Confirmed green after this. The leftovers named their source, and it had no manifest, no blob row and no files. Nothing will ever reap that hash again, so purge_derived_blobs can never fire for it — meaning the rows were written AFTER the source died, not left behind by a reap that skipped them. Two earlier attempts assumed the latter and fixed the wrong thing. Background thumbnail generation is spawned and unawaited, so an upload deleted promptly — constant in a test suite, occasional for real users — has its render finish after GC reaped the blob and then record three mappings to a corpse. Each pins its own thumbnail blob at ref_count 1, which GC is thereafter CORRECT to refuse: that is why three passes with force=true reclaimed nothing and why the leak was invisible, a healthy system declining to delete referenced data. store_derived_blob now inserts only WHERE the source still exists, checking both tables since source_hash names a Blob — a manifest for CDC content, a bare blob row for legacy whole-file content. A refused insert falls into the existing `inserted == 0` branch and releases the reference, so the thumbnail blob becomes collectible rather than stranded. Closed in both directions: if the source dies before the statement's snapshot the row is refused; if after, that reap's purge finds the row. 034f1050 stays — the bulk manifest reap genuinely lacked the purge that reap_blob had, and two manifest reap paths with only one purging is its own defect. It just was not this one. Still missing, and now clearly worth building: the orphan-mapping check the plan's coverage matrix already lists (content_derived_blobs. source_hash with no Blob behind it). This stops new ones; nothing yet finds the ones already on disk. |
||
|
Edouard Vanbelle
|
ea5d3003e0 |
fix(dedup): bulk manifest reap orphaned every derived row
Real leak, found by storage_cleanup_check.sh: three blobs surviving a full teardown, all `derived=1`, all naming one `src` whose manifest, blob row and files were already gone. The source had been reaped without its derived rows being purged. `reap_blob` purges correctly for the single-blob path. The BULK manifest reap did not — it iterated the deleted batch only to invalidate the manifest cache, so every manifest reaped that way left its content_derived_blobs rows behind. The predicate is not at fault. It protects a manifest that IS a derived artifact (content_derived_blobs.blob_hash) and deliberately not one that is the SOURCE of them, because counting source_hash as a reference would pin every original for as long as a thumbnail existed. The source is therefore reaped correctly and the purge simply has to follow it. The consequence is permanent, not cosmetic: the orphaned row holds chunk_manifests.ref_count at 1 on the thumbnail's own blob, so GC is thereafter CORRECT to refuse it — which is why three passes with force=true reclaimed nothing. Every deleted image left three behind, one per size, growing forever. Fixed at the reap rather than in any deletion path, which is where all of them converge: folder cascade, drive deletion, user deletion and single-file delete all reach it through the decrement trigger, so one call covers every route. |
||
|
Edouard Vanbelle
|
1b68ee093e |
feat(storage): both import jobs tick daily instead of manual-only
Registered with interval None, so they ran only when someone remembered to trigger them — which was your objection to gating anything on operator timing. Now daily. Not boot-time: that would delay readiness for a filesystem walk, and both jobs are idempotent and resumable, so periodic is strictly better. The tick deliberately does NOT delete. `repair` defaults false, so scheduled runs import and stop; unlinking stays a deliberate operator action, per no-silent-auto-repair. That splits the two halves the way their risk differs — the backfill is safe to automate, removing files is not. Cost once drained is a read_dir over three directories returning nothing, and after the directory itself is removed, not even that. |
||
|
Edouard Vanbelle
|
c778b67006 |
feat(thumbnails): stop writing sidecars (step 10d2)
The write paths now persist only to the blob tiers. Until this, dual-write
meant any render or upload recreated .thumbnails/ seconds after the import
job removed it, so step 10e's gate — "the directory no longer exists" —
could never hold.
Rendered thumbnails: the fs::write in persist_rendered is gone. Safe
because the read flip landed first, so nothing depended on that write to
be found, and a failed derived store now costs a re-render rather than
data — regenerable by definition. Existing sidecars are untouched and stay
readable through the fallback until the import drains them.
Uploaded previews needed a change first, and the order was not optional.
upload_thumbnail_impl logged and still returned 201 when store_attached_blob
failed — safe only while ext-{file_id}.jpg was a second copy. These bytes
have NO server-side render path, so removing the sidecar while the store
stayed best-effort would lose a user's upload behind a success response.
The PUT is now fatal, and drops the RAM entry too, or the cache would keep
serving a preview that was never persisted and vanishes on eviction,
contradicting the error the client just received. Only then does the ext-
write go.
thumb_import_check.sh had to change with it: its premise was "upload, then
delete the row, and what remains on disk is legacy state", which no longer
holds now that nothing writes sidecars. It lays them down itself, with the
bytes the API just served, at the exact paths the pre-10d2 code used. The
reconstruction stays faithful — same bytes, same paths — it just no longer
depends on current code to produce a shape current code has stopped
producing. Both sidecars get identical bytes, which is realistic rather
than a shortcut: they dedup to one blob while keeping separate mappings,
which is the property the keying split exists to preserve.
|
||
|
Edouard Vanbelle
|
df619a7ed9 |
feat(storage): thumb_attached_import drains its sidecars too
Completes step 10d. Same `?repair=true` opt-in and the same readback-before-unlink as the derived half, and the check matters more here: these sidecars hold the bytes that CANNOT be regenerated — a client-uploaded PDF preview has no server-side render path — so the read is the only thing between a migration and permanent loss, not belt-and-braces. verify_and_unlink is shared rather than copied. Two versions of "only delete after proving the replacement is readable" would be two chances to weaken one, and it is the rule the whole deletion step rests on. Deletion covers the already-imported branch as well as fresh imports, for the same reason as the derived job: a run without `repair` leaves the sidecar behind, and a later run with it would otherwise see "already imported" and never drain. Import first, enable deletion after, is the expected operator sequence, so that branch is the common path. Orphaned sidecars stay untouched — this job imports, it does not reclaim, and a destructive default on a migration is what no-silent- auto-repair forbids. Unverifiable ones are kept and reported, so the next run retries. With both halves draining, `.thumbnails/` can now actually empty and the directory removal in the derived job can succeed — though not stably until dual-write stops, since any render or upload recreates it. |
||
|
Edouard Vanbelle
|
ae9ff0d8b3 |
feat(storage): thumb_derived_import drains the sidecars it imports
Step 10d, for the content-keyed half. Opt-in via the existing `repair` flag rather than a new one — the house rule is that a job does not mutate on its default setting, so early runs import only and an operator can inspect before committing. Deleting from the job rather than a later release is what makes the migration self-draining. Sidecars are LOCAL disk, so no release can know whether every instance has finished; each instance draining itself needs no coordination at all. Verification before unlinking is the load-bearing part. store_derived_blob reporting success is not proof the bytes are retrievable — a backend that accepted a write it cannot serve would otherwise have the last copy deleted on top of it. The blob is read back and its length compared against the sidecar's; a failure keeps the file, records a finding, and the next run retries. That read is the difference between a migration and a data-loss bug. Deletion applies to already-imported files too, not just fresh ones. A run without `repair` leaves the sidecar behind, and a later run with it would otherwise classify the file as "already imported" and never drain it — and import-then-enable-deletion is the expected operator sequence, so that is the common path rather than an edge case. Directories are removed once genuinely empty, because ABSENCE is what step 10e gates on, not emptiness: empty is momentary and an on-demand render can repopulate it a second later, while absence is one-way and cheaper to test (one stat, versus opendir/readdir/closedir). remove_dir refuses a non-empty directory, so it needs no emptiness check and cannot race a concurrent write into deleting live files. thumb_attached_import still needs the same treatment; verify_and_unlink should move somewhere shared rather than being copied into it. |
||
|
Edouard Vanbelle
|
12158ccf59 |
fix(storage): thumb_derived_import claims JPEG sidecars too
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.
|
||
|
Edouard Vanbelle
|
f4e4b518a9 |
feat(thumbnails): classify render failures as permanent or transient
Prerequisite for persisting negative verdicts, and the reason step 7 cannot start with the storage change. generate_and_persist collapses EVERY error into empty Bytes — timeouts and closed semaphores included. That is survivable only because the sentinel lives in moka, which evicts. Write the same signal to content_derived_blobs and a thumbnail that timed out once under load is unrenderable forever. So the classification lands first, on its own, before anything can persist it. It maps onto the existing variants without restructuring, because timeouts already surface as TaskError: ImageError, UnsupportedFormat -> permanent. The decoder rejected these bytes, or they exceed MAX_DECODE_PIXELS. Facts about the image. TaskError, IoError -> transient. Timeout, closed decode semaphore, join failure, unreadable source. Facts about the moment. The asymmetry sets the default: a wrongly-persisted transient marks a good image unrenderable for good, while a wrongly-omitted permanent merely costs a repeated decode. Anything not clearly a content property is therefore transient. Tested by pinning the mapping rather than trusting variant names to stay put — the timeout case especially, since it is the one that turns a load spike into data loss. Nothing consumes this yet; it exists so the storage change cannot be written without it. |
||
|
Edouard Vanbelle
|
d202b4b5ca |
fix(storage): the derived import conflated variant with directory
76590160 changed `variant_of` to return `{size}.{ext}`, but that value
was also being used as the on-disk DIRECTORY. Reads became
`.thumbnails/preview.webp/{hash}.webp`, which does not exist, so every
sidecar counted as unreadable and thumb_derived_import restored nothing.
Caught by thumb_import_check.sh on the run after the migration — the
harness earning its keep twice now, since this is the second defect it
has caught that no unit test could.
They are genuinely two strings and are now named as such: `dir_name` for
the path, `variant` for the row key. The cursor keeps using the
directory, so a run paused before the migration resumes at the same
position rather than restarting.
Also stops podman's compose-provider banner from burying the script's
output. Filtered rather than discarded, so genuine psql errors still
surface — swallowing those would turn a broken query into a silently
wrong assertion. Suppressing it at the source needs
`[engine] compose_warning_logs = false` in containers.conf, which is
per-developer config and cannot be relied on in CI.
|
||
|
Edouard Vanbelle
|
86d0d65583 |
feat(storage): derived variant encodes the output format
`content_derived_blobs.variant` held the size alone, so one source could
hold exactly one artifact per size regardless of codec. That surfaced
when the read order flipped in 10c: a JPEG request matched the WebP row
and would have been served the wrong codec — hidden previously because
the .jpg sidecar won first. The flip had to be gated to WebP, which
meant JPEG clients could never leave the sidecar, which meant the
sidecar could never be deleted.
It blocks transcodes harder: those are multi-format by nature, so two
output codecs of one source collide on the primary key without a format
term.
The axis goes inside the string rather than into a fourth PK column,
per the column's own rule — "new axes go inside this string, never into
new columns". Shape is {size}.{ext}: preview.webp, icon.jpg, later
720p.webp.
The backfill is deterministic, not a guess: store_derived_blob has only
ever written "image/webp" for thumbnails. content_type is checked anyway
rather than assumed — a row that fails the assumption is left alone and
counted in a warning, because the read path then simply misses it and
falls back to the sidecar, whereas guessing a codec would serve wrong
bytes. Idempotent via NOT LIKE '%.%', so a re-apply cannot produce
preview.webp.webp; verified on a scratch PG by applying it twice.
One helper builds the string, because it is a primary-key component:
a writer and reader that disagree do not fail loudly, they just never
find each other's rows and the derived tier silently looks empty. It
lives on the service's ThumbnailSize, not the port's — they are distinct
types, which the compiler pointed out after I put it on the wrong one.
The WebP gate on the read path is now removed: each codec has its own
row, so JPEG can finally reach the derived tier — the prerequisite for
deleting the sidecar for those clients.
file_attached_blobs keeps a bare size: store_external_thumbnail
re-encodes everything to JPEG, so it is single-format by construction
and a format term would cost a migration for nothing.
|
||
|
Edouard Vanbelle
|
c656e684d4 |
feat(consistency): merge-join backend_consistency, both directions
The job walked the backend and probed the DB with WHERE hash = ANY($1) over each page, so it could only ever see backend-only entries. A registry row whose bytes are gone never appears in a backend listing — it was invisible here by construction, and that half was left to blobs_consistency's per-row HEAD probe, which does not survive the row counts this plan produces. Both sides are now ordered by hash — the backend by contract since 5343fdda, the DB by ORDER BY hash — so one pass yields both deltas: orphan_blob for bytes with no row, and blob_missing_from_backend for a row with no bytes. The latter is severity data_loss rather than inconsistent: an orphan wastes space, this loses a file. Three things the merge needs that a probe did not. A horizon. The two pages cover different ranges, so only their overlap can be judged — beyond it, a hash missing from one side may simply be on the next page of the other, and emitting there would invent findings in both directions at once. When a side is exhausted its entries cannot be on a later page, so the other's tail becomes judgeable. One cursor for both sides. They share an ordering, so "resume after H" is start_after(H) on the backend and hash > H in the DB. The cursor advances to the horizon, not the backend's own next_cursor, which would skip the un-judged tail of whichever side reached further. Format is unchanged, so paused runs resume. And an ordering premise worth stating rather than assuming: hashes are lowercase BLAKE3 hex of fixed length, so collation and byte order rank identically over [0-9a-f]. A hash column admitting uppercase or variable length would break this silently. Recorded in the module docs and beside the query. blobs_consistency still emits its own blob_missing_from_backend; the two now overlap. Retiring that probe is the follow-up, not folded in here. |
||
|
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. |
||
|
Edouard Vanbelle
|
48d9e164d4 |
test(thumbnails): pin get_cached_thumbnail's tier precedence
This function produced four bugs in two days, every one an ordering mistake rather than a logic error, and every one caught only by an end-to-end run comparing bytes against something independent: the content-keyed RAM entry shadowing an uploaded preview so a PUT appeared to do nothing; that precedence being right on disk but wrong in RAM; a validator flipping because a tier was populated as a side effect of producing the body; a decode error reading as "absent". They all violate one sentence — a file-specific override beats anything derived from the content, at every tier — so that is what these pin. Step 10c is entirely a precedence change (derived ahead of sidecar), and it should not be another end-to-end guess. No database needed. With `dedup: None` the two DB tiers are skipped, and what remains — per-file RAM, ext- disk, content RAM, blob-hash sidecar — is exactly where the bugs were. Seven cases: the two override rules, RAM over disk within the content tiers, the sidecar answering alone, the ext- read caching under the PER-FILE key (a content key would leak one user's preview to every file sharing the content), a hashless caller falling through instead of guessing, and moka's empty-bytes negative entry not being served as a thumbnail. Verified by mutation, not just by passing: restoring the old precedence fails exactly the two tests that encode the rule, and no others. Lives in thumbnail_service.rs rather than the sibling test file because seeding tiers needs the private `cache` field. |
||
|
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. |
||
|
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. |
||
|
Edouard Vanbelle
|
7ec387003d |
refactor(thumbnails): one persist_rendered for every render path
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.
|
||
|
Edouard Vanbelle
|
395296a7e7 |
fix(storage): file_exists misreported every file as missing
`SELECT 1 FROM storage.files WHERE id = $1` decoded as i64. PostgreSQL types a bare `1` as int4, so the decode always failed — and since `.ok().flatten()` turns a decode error into the same None as "no row", file_exists reported false for every file. thumb_attached_import therefore classified every sidecar as an orphan and imported nothing. Caught by thumb_import_check.sh on its first run: the derived import restored its rows, the attached one restored none. Now `SELECT EXISTS(...)`, which yields a real bool and always returns exactly one row, so absence means absence. A query error still degrades to false — the safe direction, leaving the file on disk as a reported orphan rather than importing it against a row that may not exist. The failure mode is the point, and it is the third of this shape in two days: an error converted into an innocuous-looking outcome. So the check script now dumps a job's findings when an assertion fails. The jobs already recorded exactly why they skipped each file — the attached_sidecar_orphan findings naming the cause were sitting in the run while the script reported only "did not restore the row", which is indistinguishable from the job never having run. |
||
|
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.
|
||
|
Edouard Vanbelle
|
49e7bb15a6 |
test(storage): cover the sidecar walk for both import jobs
Both imports run over ONE directory, where the two legacy shapes sit side by side, so the property worth asserting spans them: together they must claim every real sidecar exactly once, and neither may take the other's. A job that drifted into the other's shape would content-key user-supplied bytes — sharing one user's uploaded preview onto every file with identical content — and no per-job test in isolation would notice. So the fixture is shared. `legacy_tree` builds a directory holding a content-keyed .webp pair, an ext- upload, and a stray README, and both test modules walk it: derived claims exactly the two hashes in sorted order, attached claims exactly the ext- file, the two sets are disjoint, and between them they account for all three real sidecars. `sidecar_names` became an associated function taking the root instead of reading `self`, which is what makes this testable at all — the walk is the half that decides which files a job claims, and it needed no pool to verify. Sorting is asserted rather than assumed, since the cursor resumes by skipping everything at or before it and a stable order is the only thing that makes that correct. A missing size directory is covered too: normal on a fresh install, and it must yield no work rather than abort the walk. Not covered here, and it needs a pooled fixture that does not exist: the round trip itself — store the blob, write the row, and confirm a COPY inherits the preview. That belongs in the API-level harness, where the legacy state can be manufactured through the real write path and then stripped. |
||
|
Edouard Vanbelle
|
7a2ebe0fdc |
feat(storage): thumb_attached_import — backfill uploaded previews
Twin of thumb_derived_import, for the other sidecar shape:
{thumbnails_root}/{size}/ext-{file_id}.jpg, the previews a user
uploaded — notably the SPA's client-side PDF generator, which has no
server-side render path at all.
Until a row exists, a copy of the file LOSES the preview: the sidecar is
keyed by file_id, no copy path duplicates it, and the server silently
falls back to rendering from the source, or to nothing for a PDF. That
is the bug file_attached_blobs closed for new uploads; this closes it
for everything already on disk.
Separate job rather than an arm of the derived import, because the
keying differs and that difference is the security boundary. These bytes
are not derivable from the file's content, so content-keying them would
share one user's uploaded preview onto every file with identical
content. Each job's name filter rejects the other's shape, and both
directions are under test.
Idempotence needs more care here than in the derived twin.
store_attached_blob is ON CONFLICT DO UPDATE, so calling it for an
existing row releases the previous reference and takes a new one —
harmless once, but a job doing it every run would churn refcounts. The
row is therefore checked first and the store reached only on a genuine
insert.
uploaded_by is the nil sentinel: disk records no uploader, and inventing
one — the file's created_by, say — would fabricate provenance that could
later read as evidence an Editor replaced someone's preview. The column
is NOT NULL with no FK precisely so provenance survives, and a sentinel
says "unknown" honestly.
Orphaned sidecars (no storage.files row) are counted and reported, not
deleted. This job imports; it does not reclaim. Existence is checked
explicitly rather than letting the foreign key reject the insert, so an
orphan is counted as one instead of surfacing as an opaque constraint
error.
|
||
|
Edouard Vanbelle
|
f80a28763e |
feat(storage): thumb_derived_import — backfill the derived tier from sidecars
First half of step 10. Every server-rendered thumbnail written before
content_derived_blobs existed lives only as
{thumbnails_root}/{size}/{hash}.webp — local-disk state that another
instance cannot see, a backend migration does not carry, and no
consistency job covers. This walks those files into the blob store and
records the mapping, so the derived tier can become authoritative and
the sidecar can be deleted.
A registered JobRegistry tenant rather than a script: the volume is
unbounded, so it needs a cursor, resume, cooperative cancel and run
history, and an operator needs somewhere to watch it. Cursor is
{size_dir}/{filename} over a sorted walk, which totally orders the
traversal.
Idempotent by construction — each file is skipped when a row already
exists, and store_derived_blob is ON CONFLICT DO NOTHING with
release-on-conflict beneath it, so re-runs cannot inflate refcounts.
Re-running is the expected operator behaviour, since Phase 3 (deleting
the sidecars) is gated on a run reporting zero imported.
hash_from_sidecar_name deliberately rejects ext-{file_id}.jpg. Those
bytes are user-supplied and file-keyed; importing them here would
content-key them and share one user's uploaded preview onto every file
with identical content. They belong to thumb_attached_import. Both the
accept and the reject set are under test.
Unreadable files and store failures record a finding and continue: a
sidecar removed by a concurrent GC unlink between listing and read is
expected, not fatal, and the file is left in place for the next run.
Registered unconditionally rather than behind a flag — a migration
nobody can find is a migration nobody runs.
|
||
|
Edouard Vanbelle
|
7d9418f63c |
fix(thumbnails): ETag names the blob actually served
fe9c4f49 keyed the ETag on the SOURCE file's content hash. That is wrong
whenever the response comes from a satellite table, and for attachments
it is wrong in two ways.
Uploading a preview does not change the file's content, so a
source-keyed ETag does not change either — and with `immutable` set,
clients never revalidate and keep the previous render for up to a year.
The exact staleness fe9c4f49 set out to fix, re-entering through the
attachment path.
Worse: a copy inherits the source hash, so an original and a copy have
identical ETags. Give either one a different uploaded preview and they
serve different bytes under one validator, which a shared cache may hand
to either request. That is a collision, not just staleness.
thumbnail_content_id resolves the identity through the same tier
precedence the read path uses: an attached blob's own hash, else a
derived blob's own hash, else the source-keyed form. An ETag naming a
different tier than the one answering is worse than a coarse one, so the
two orders must not drift.
Derived-hash keying is strictly better than source-keying and never
worse. The sidecar and the derived row are written from the same bytes;
where they can diverge — a sidecar re-rendered while the derived row
stays pinned by ON CONFLICT DO NOTHING — source-keying is wrong too,
because the renderer is not part of that key. This is the step 10 change
arriving early, forced by the attachment case; the plan note stands for
the read-order flip itself.
Known gap: a legacy ext-{file_id}.jpg with no file_attached_blobs row
yet falls through to the source-keyed form. No worse than today, and it
resolves when the import backfills.
attached_thumbnail_copy.hurl now asserts ETags, which is why this went
unnoticed: it compared bytes only, and thumbnail_etag_content_keyed
covers content replacement rather than preview upload. A fresh GET
returned the right bytes throughout — the same "healthy locally, broken
for anyone caching" shape as the two bugs before it.
|
||
|
Edouard Vanbelle
|
d71dd973e7 |
fix(thumbnails): two defects the copy test exposed
1. Per-file overrides must beat the content tier in RAM as well as on
disk. The content-keyed lookup ran first, so a thumbnail already
rendered from the file's content sat in RAM under content(blob_hash)
and shadowed a preview uploaded afterwards — permanently. Invisible
before the moka rekey, because both lived under one file-id key and
the upload simply overwrote the render. Order is now uniform:
per-file RAM, per-file disk (ext-), per-file DB, then content RAM,
blob-hash disk, derived blob.
2. store_attached_blob never wrote a row. Its RETURNING clause compared
the stored hash against EXCLUDED, and PostgreSQL only permits
EXCLUDED in the SET and WHERE of DO UPDATE — a runtime syntax error
on every call. The superseded hash now comes from a SELECT taken
before the upsert; losing that race leaves one stale reference, which
the manifest recompute reports, rather than anything being lost.
The second hid behind the first for a whole cycle, and behind
`ext-{file_id}.jpg`: the ORIGINAL kept serving its uploaded preview from
local disk, so the feature looked healthy. Only a copy, which has a
different file_id and therefore no ext- file, depends on the row — and
the row was never there. The handler's best-effort warn! completed the
disguise, so it is now error!: a failure there means copies silently
lose the preview, and nothing else signals it.
|
||
|
Edouard Vanbelle
|
64ff982571 |
feat(thumbnails): uploaded previews survive a copy
Completes step 9. The PUT wrote `ext-{file_id}.jpg` and nothing else —
keyed by file id, on local disk. No copy path duplicates it and no other
instance can see it, so a copied file lost the preview its owner
uploaded. Silently: the server falls back to rendering one from the
source, or to 204 for a PDF, which has no render path at all. A
user-supplied preview is not derivable from the content, so once lost it
is gone.
The PUT now also records a storage.file_attached_blobs row, which
copy_file_satellites already duplicates, so both copy paths carry it.
Best-effort: the sidecar has already succeeded by then and the user can
see their thumbnail, so failing the request would report an error for an
operation that visibly worked.
Read path consults attachments ahead of every content-derived tier: an
uploaded preview is an explicit choice about THIS file and must beat
anything rendered from its content. Cached under the per-file key — a
content key would leak those bytes to every other file sharing the
content, which is the poisoning the file-keyed table exists to prevent.
store_attached_blob is ON CONFLICT DO UPDATE, unlike its derived twin:
re-uploading a preview is a deliberate replacement, where a re-derived
thumbnail is the same bytes again. The superseded blob's reference is
released, or it would be pinned forever with nothing pointing at it.
Deletion goes through a trigger, not a hook. file_id is ON DELETE
CASCADE, and on_file_deleted fires AFTER delete_file — by then the
cascade has run and there is nothing left to enumerate. This matters
most for folder deletion, where PG cascades folders to files to
attachments and Rust never sees the rows at all. storage.decrement_blob_ref
keys off OLD.blob_hash and is otherwise table-agnostic, so it is reused
verbatim rather than transcribed into a second trigger that can drift.
DELETE only: a replacement updates in place and is handled in Rust, so
adding UPDATE would double-decrement.
Extracted read_blob_to_bytes, shared by the attached and derived tiers —
the only difference between them is which table produced the hash.
tests/api/attached_thumbnail_copy.hurl guards it. The file is red and
the uploaded thumbnail is green, so a render could never produce the
uploaded bytes; the pre-upload render is captured first and required to
change, which stops three identical renders from satisfying the
byte-equality. Then both copy paths must serve the upload, and after the
original is purged and GC runs, both copies must still serve it — each
holds its own reference, because the rows are duplicated rather than
shared.
|
||
|
Edouard Vanbelle
|
fac82fea23 |
feat(storage): add storage.file_attached_blobs, the file-keyed half
Step 9 of docs/plan/derived-blobs.md. content_derived_blobs holds bytes that are a pure function of a file's content, so they are keyed by that content and shared by every file holding it. This table holds the opposite: bytes a user supplied or chose, which must never be shared across files. The key is what enforces it. That difference is a security boundary, not a modelling preference. A content-keyed client preview would let user A upload a file plus a preview that misrepresents it; when user B later uploads the same bytes, dedup matches and B is served A's preview. Content-keying is only safe when the server can derive the bytes — there is nothing to poison, because the same input yields the same output for everyone. Required now rather than deferred: the SPA already generates and PUTs previews for PDFs, and there is no server-side regeneration path for them, so the sidecar migration has nowhere else to put those bytes. uploaded_by is NOT NULL with no foreign key, per the provenance convention rather than the plan's sketch. A FK with ON DELETE SET NULL discards the audit trail exactly when it matters, and without an ON DELETE clause it would block deleting a user outright. Deleting the uploader must not rewrite history. FileAttachedReferenceSource is registered in built_in_registry before anything writes to the table, so dedup_gc's reap predicate already knows it exists — otherwise the first sweep after the first attachment would delete it. Manifest level only, like the derived source: these blobs are almost always single-chunk, so contributing at chunk level would double-count against the aliased hash. copy_file_satellites gains one arm: attachments are DUPLICATED, since the key is file_id and the copy is a different file, with uploaded_by carried over — the person who supplied the bytes did not change because someone copied the file. Each duplicate takes its own reference, so the bytes stay deduplicated while the mapping does not. Both golden SQL tests updated: the new fragment lands inside the reap predicate's NOT(...) group and as a summed term in the manifest recompute. Verified on a scratch PG with every migration applied — the attachment duplicates to 2 rows holding 2 references with provenance intact, while the content-keyed thumbnail stays 1 row reachable from both files. |
||
|
Edouard Vanbelle
|
cf21ee2f8f |
fix(thumbnails): key the moka tier on content, closing the ETag race
fe9c4f49 made the ETag content-keyed, but the RAM tier was still keyed on
file_id, so the two disagreed about what identifies a thumbnail. Replacing
a file's content preserves its id, so the moka entry stayed reachable while
the ETag had already changed — and invalidation runs from the spawned task
in on_file_updated. A request landing in that window got the NEW ETag over
the OLD bytes, and because the response is immutable with a one-year
max-age, the client cached those stale bytes permanently. The bug fe9c4f49
set out to fix, arriving through a different door.
Keying on the hash removes the window rather than narrowing it: new content
is a different key, so the old entry cannot be hit. Correctness no longer
depends on the invalidation task winning a race against the next request.
This also aligns the RAM tier with what disk already did — sidecars have
always been written to get_thumbnail_path(blob_hash, ...). The tier that
had the bug was the one keyed differently from every other. Two further
consequences: N copies of one photo now share a single entry instead of
occupying N for identical bytes, and delete_thumbnails shrinks to the
external entries, which are the only genuinely per-file artifacts.
Video frames stay file-keyed under an `ext-{file_id}` id — they are
per-file by nature. The namespaces cannot collide: hashes are 64 hex
characters.
get_cached_thumbnail takes blob_hash as an Option, and a caller without one
now skips the RAM tier and falls through to disk rather than consulting a
file-id key. That is correct, not merely tolerable — a file-id key is the
stale entry this change exists to prevent. Both HTTP handlers resolve the
hash to build the ETag, so only internal callers that never had one are
affected.
|
||
|
Edouard Vanbelle
|
7261b5b175 |
fix(s3): never return a mangled enumeration cursor
5343fdda switched S3 blob enumeration from an opaque continuation token to a hash cursor (StartAfter), per the port contract. Its fallback for a page containing no canonical blob was wrong: it stored the full key (`0a/junk.tmp`), stripped it to a basename (`junk.tmp`), and the next call fed that to `object_key()` — producing `ju/junk.tmp.blob`. Wrong shard and a doubled extension, so the resume jumped to an arbitrary position: skipped objects, or backwards into a loop. The cursor can only ever be a real hash, because `object_key()` is applied to it. So instead of synthesising one, keep listing internally until the page holds at least one blob or the bucket is exhausted. The continuation token is used only inside the call and never escapes. Two pathological cases cannot produce a cursor at all — `is_truncated` with no token (protocol violation), and a run of foreign keys long enough to buffer the bucket. Both now fail loudly. A visible job failure beats a sweep reporting "no missing blobs" having read a fraction of them. Extract `hash_from_object_key` as the paired inverse of `object_key`, with the round-trip and the rejection set under test. It also now requires the shard to match the hash's own prefix, which the inline filter did not check. |
||
|
Edouard Vanbelle
|
7f5ee7401f |
refactor(storage): make blob enumeration ordered and hash-cursored
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> |
||
|
Edouard Vanbelle
|
c276d3861e |
fix(thumbnails): release derived blobs when their source is reaped
Caught by the api-test storage check: 15 blob files left on disk after a full cleanup. Since 3736b577 thumbnails are stored as derived blobs, and each content_derived_blobs row holds a manifest reference — but nothing ever deleted those rows, so the reference outlived the source and GC could never reclaim the bytes. The plan specifies this cascade; I implemented the write and read paths and missed it. Adds `purge_derived_blobs`, the delete counterpart of `store_derived_blob`: deletes every row derived from a source hash and releases the reference each held. It lives on DedupService alongside its store/find siblings because ThumbnailService cannot hold a DedupService — it implements BlobLifecycleHook, and holding one would close the DedupService -> BlobLifecycleService -> hook -> DedupService cycle the existing comment warns about. All five reap sites now go through `reap_blob`, which purges then fires the lifecycle hooks, so no path can drop a blob without first releasing what was derived from it. Previously each site called fire_blob_hooks directly, which only cleaned the sidecar files ThumbnailService owns. `reap_blob` is boxed because it is mutually recursive with `remove_reference`: releasing a thumbnail's reference can reap the thumbnail's own blob, which re-enters here. It terminates after one level — nothing is derived from a thumbnail, so the inner purge finds no rows. That bound is a property of the data, not an invariant the code enforces, so it is stated at the definition. fmt, clippy --all-features --all-targets, unit tests clean. The api-test storage check is the real verdict — it is what found this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edouard Vanbelle
|
60b94e1183 |
feat(thumbnails): serve derived blobs when the sidecar cannot
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>
|
||
|
Edouard Vanbelle
|
1c488b7df5 |
feat(thumbnails): also store derived thumbnails as blobs
Step 5, write path only. Every eagerly-rendered thumbnail is now ALSO
stored through DedupService and recorded in
storage.content_derived_blobs. The sidecar write stays and reads are
untouched, so nothing user-visible changes.
That split is deliberate. This is the first commit in the plan that
changes runtime behaviour on a hot path, so it fills the table while
reads still come from disk: the rows can be inspected against real data
before anything depends on them, and a rollback at any point leaves
working thumbnails. The read path and sidecar removal follow separately.
DedupService::store_derived_blob does the whole contract in one place,
so no caller has to remember the accounting:
* writes the bytes through the normal CDC path, so derived blobs
inherit the backend, encryption, migration and key rotation that
source content already gets;
* records (source_hash, kind, variant) -> blob_hash;
* releases the reference store_from_stream took IF the mapping
already existed. Two instances racing to render the same thumbnail
must leave ref_count at 1, not 2 — otherwise every re-render
inflates it and pins the blob forever.
ThumbnailService deliberately does NOT gain a DedupService field: it
implements BlobLifecycleHook, and holding one would close the cycle
DedupService -> BlobLifecycleService -> hook -> DedupService that the
existing comment warns about. The handle is passed per call instead,
which every eager path already has.
The tier-3 write is best-effort and logged. A failure must not cost the
user a thumbnail that is already on disk and in the moka cache;
`derived_import` sweeps anything missed. The sidecar write keeps its
existing failure behaviour and now `continue`s, so a disk failure no
longer falls through to the cache insert.
Nothing reads these rows yet, so the only observable effect is rows
appearing in the table and the manifest ref_count they hold — which
`manifests_consistency` will now count, since
ContentDerivedReferenceSource was registered in 8d4052e1 before any
writer existed.
fmt, clippy --all-features --all-targets, and 35 unit tests across the
touched modules clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Edouard Vanbelle
|
a7938344dd |
feat(storage): add content_derived_blobs table + reference source
Step 5 foundation of docs/plan/derived-blobs.md. Creates the mapping
table for server-derived artifacts and registers it as a blob-reference
source — deliberately BEFORE anything writes to it, which is the
ordering the plan requires: dedup_gc's reap predicate has to know the
table exists, or the first sweep after the first thumbnail deletes it.
No writer yet, so this is inert: the table is empty and every added SQL
term counts zero. The point is that the machinery is in place first.
storage.content_derived_blobs maps (source_hash, kind, variant) to the
derived blob_hash. The two hash columns mean different things and the
migration says so at length: source_hash is a DEPENDENT pointer holding
no reference (the file keeps the source alive), while blob_hash is a
reference HOLDER bumping chunk_manifests.ref_count. Counting source_hash
would pin every source Blob for as long as a thumbnail existed.
ContentDerivedReferenceSource contributes at the manifest level only.
A derived artifact's blob_hash names a Blob, never a chunk, and
contributing at the chunk level would double-count — a thumbnail is
almost always single-chunk, so its manifest hash equals its lone chunk's
hash, the same aliasing trap the legacy-files term guards against with
NOT EXISTS. There is a test for the invariant, and the chunk-level
golden test passing UNCHANGED is independent confirmation.
Collapses three definitions of "what references a blob" into one.
Adding the source revealed that DI assembled its own registry while
DedupService::new built a different default, and the two consistency
test helpers built a third — so the golden tests would have pinned SQL
production never runs. There is now a single `built_in_registry(pool)`;
DI reads it back via DedupService::reference_registry() rather than
assembling its own.
The reap-predicate golden test caught the change exactly as designed,
and the new branch landed inside the NOT (...) group ORed with files —
so a manifest is reaped only when NEITHER source references it. A branch
landing outside that group would have inverted the predicate for every
other source; that is why the test pins the whole statement rather than
asserting substrings.
fmt, clippy --all-features --all-targets, and 15 unit tests clean.
fix(migrations): order content_derived_blobs after the refcount fixes
Renames 20261015000000_content_derived_blobs.sql to
20261018000000_content_derived_blobs.sql.
The file was authored before the rebase onto fix/copy_folder_ref_count_issue,
so its version sorted BEFORE migrations that now precede it in history:
20261016000000_copy_folder_tree_manifest_refcount.sql
20261017000000_file_delete_trigger_manifest_aware.sql
20261017000002_repair_existing_refcount_drift.sql
Filename order and commit order disagreeing is the problem, not any
dependency — the table is standalone and creates nothing those
migrations touch. But an installation that has already applied through
…17000002 would then be offered a LOWER unapplied version, which sqlx
either applies out of order or rejects on its version check, and a
fresh install would get an ordering no upgrade path ever produces.
Reproducibility between the two is the whole point of the version
prefix.
Kept as its own commit rather than amending 01d90524, since interactive
rebase isn't available here and rewriting mid-branch while the ref_count
work is still being rebased elsewhere would churn hashes again. Worth
squashing into 01d90524 at merge.
No content change — pure rename, verified nothing references the old
filename.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Edouard Vanbelle
|
390aa31443 |
feat(cli): merge oxicloud binary and cli
this feature to simplify the creation of only 1 binary for multiple architecture |
||
|
Edouard Vanbelle
|
fd103c46e6 | feat(manifests-consistency): add safe repair mode | ||
|
Edouard Vanbelle
|
0f29614a3b |
fix(ref_count): use SQL to correct ref_count on cascading deletion
then dedup_gc will trigger blob life cycle and ensure chunk deletions |
||
|
Edouard Vanbelle
|
7e8029027e |
fix(dedup): log the manifest reap predicate at info, not debug
The reap statement is assembled from the registered reference sources, so it cannot be grepped out of the source tree — and it DELETES manifests. Hiding it behind a debug filter an operator has to know to enable was the wrong default: if what GC considers "referenced" ever changes, that has to be visible on the next boot without anyone going looking for it. Reported in testing: `RUST_LOG=info,oxicloud::dedup=debug` did not surface it, while a global `RUST_LOG=debug` did — at the cost of an unusably noisy boot. Rather than have operators carry a special filter for a line describing a destructive statement, promote it. The statement is whitespace-collapsed into a single `statement` field so a multi-line query does not sprawl across the boot log, and the registered `sources` are logged alongside it — that list is what actually determines the predicate, so a change to it is the thing worth noticing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edouard Vanbelle
|
213e1c553a |
feat(consistency): reconcile chunk_manifests.ref_count
Step 3 (prerequisite 2) of docs/plan/derived-blobs.md, and the last one
before the thumbnail slice.
There are two reference counters and only one was ever verified.
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: chunk references feed storage.blobs and are reconciled
by blobs_consistency::refcount_mismatch, while Blob references — every
CDC file, and every derived artifact once those exist — feed
chunk_manifests.ref_count, which nothing reconciled.
That gap was survivable only because dedup_gc's reap predicate carried a
second clause ("no storage.files row references this manifest") that
quietly compensated for drift on the bulk-delete paths where ref_count
is never decremented. Generalising that clause to the reference registry
in 1c8ead49 — so thumbnails stop being reaped — removed the
compensation, which is precisely why the counter now needs checking
directly. The two changes have to ship together.
Adds manifests_consistency, a recoverable job reporting
manifest_refcount_mismatch (severity inconsistent). The finding carries
reap_risk so an operator can triage: an under-count means GC reaps a
manifest whose content is still reachable, taking its chunks with it,
while an over-count merely pins storage.
A separate job rather than a second phase of blobs_consistency: one
subject per job, as the other five consistency tenants do, and it avoids
changing the cursor format of an existing recoverable job — which would
strand any run paused across the deploy.
The page query is assembled from the same registry dedup_gc reaps from
(via DedupService::reference_registry), built once at construction, and
pinned by a golden test. Two invariants the test guards: the files term
carries no NOT EXISTS guard — that guard keeps CDC rows out of the
*chunk* level and here would count nothing — and chunk_hashes appears
nowhere, since a manifest citing its own chunks is not a referrer of
itself.
fmt, clippy --all-features --all-targets, and 3 new unit tests clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Edouard Vanbelle
|
f658e55751 |
refactor(storage): drive blobs_consistency refcount from the registry
Completes step 1 of docs/plan/derived-blobs.md. The chunk-level `actual_ref_count` recompute was two correlated subqueries written inline; it now sums the registered reference sources instead, so `blobs_consistency` and `dedup_gc` answer "what references this hash" from one place. If they ever diverged the sweep would bless counts the collector disagrees with — and the collector wins, destructively. No behaviour change: the generated expression is the same legacy-files term (guarded by NOT EXISTS) plus the same manifests-citing-this-chunk term, and a golden test pins the whole statement byte-for-byte. Built once at construction, like the reap statement, so the sweep runs a fixed query per page rather than assembling SQL inside the loop. The builder refuses an empty registry rather than emitting a query where every blob looks unreferenced and the entire table reports refcount_mismatch; there is a test. DI now constructs one registry and hands the same instance to both consumers — `DedupService::reference_registry()` is what `BlobsConsistencyCheck` receives, so agreement is structural rather than a convention someone has to maintain. The long comment explaining the single-chunk double-count trap moved from the query site to the builder's doc comment, where the NOT EXISTS guard it describes actually lives. fmt, clippy --all-features --all-targets and the 17 affected unit tests all clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edouard Vanbelle
|
a68c938400 |
fix(storage): stop dedup_gc reaping manifests held only by new sources
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>
|
||
|
Edouard Vanbelle
|
20e6e05bb4 |
feat(sessions): identify online sessions (connected users)
identify online session by writing the `last_seen_at` information is stored in a map and flush each 30s to prevent performance impact on pgsql |
||
|
Edouard Vanbelle
|
a34da40ce9 | feat(sessions): clean expired sessions (exp > 3month) |