Adds `GET /api/admin/transcode/stats` and a hurl scenario that uses it
to assert both halves of the caching contract.
The endpoint exists because the property was previously unobservable.
`derived_blob_copy.hurl` records the same limitation for thumbnails:
stored blob, RAM cache and a fresh re-render return identical bytes
with identical status, so no HTTP-level assertion can tell them apart.
Counters can. `transcodes` is work done; `cache_hits` and `disk_hits`
are work avoided, and a rising `transcodes` against a flat `disk_hits`
is exactly what a broken derived tier looks like from outside.
Each case uploads the same bytes as TWO distinct files. Re-fetching one
file would only prove moka works — that cache is keyed `{file_id}:{ext}`.
A second file with identical content is a guaranteed memory miss but the
same content hash, so avoiding a transcode there can only be the
content-keyed tier answering. That is the whole point of keying
derivations by content rather than by file, and this is the first test
that can see it.
The negative half is the one the row exists for: without it the server
re-runs a full decode + encode of a half-megabyte screenshot for every
file sharing that content, on every request, to discard the result each
time.
Assertions capture-then-compare rather than computing deltas — hurl has
no arithmetic in predicates, and pinning the exact prior value is
stricter anyway, since a transcode triggered from anywhere shows up.
Absolute values are never asserted: other scenarios in the same run
transcode too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The transcode negative path — "the result came out larger, serve the
original and remember that" — had no test because no synthetic image
reaches it. Measured against the real encoder: flat colour goes
4780 → 186 bytes, a diagonal gradient 24852 → 102, and uniform RGBA
noise still loses by ~242 bytes at every size, a margin constant in
absolute terms and so one that never flips. Grayscale does not help
either; WebP's subtract-green transform handles R=G=B.
Two things have to be true at once and only real content does both.
The encoder is the `image` crate's own minimal VP8L writer, not
libwebp, so it wins only where redundancy is extreme enough for any
encoder to find it. And the original has to be near PNG-optimal, which
a screenshot from a real capture tool is: a 2x Retina UI is long
identical runs, flat panels and sharp edges — precisely what PNG's
scanline filters plus zlib were built for.
So the fixture is a real OxiCloud screenshot (emails masked by
overtyping rather than block-filling, which would have added back the
flat redundancy the property depends on; re-verified negative after
masking, 556180 -> 511124 bytes).
`fixture_premise` pins both halves of what tests/api/transcode_cache.hurl
will assume — this one negative, red-image.png positive. Without the
guard a future encoder bump would silently turn the negative half of
that scenario into a second positive test: still passing, no longer
checking what it was written to check.
Worth recording for whenever libwebp replaces this encoder: most of
these screenshots would likely flip to positive, which leaves every
stored negative row a stale verdict. An encoder change has to purge
them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The admin panel had no repair toggle wired to anything but a hardcoded
name list naming the two refcount tenants, so `thumb_derived_import` and
`thumb_attached_import` could not be run in repair mode from the UI at
all despite supporting it. And nothing in the job list said what any
given job does or whether clicking Run on production writes anything.
Three defaulted methods on `JobHandler` and `RecoverableJobHandler`:
fn description(&self) -> &'static str
fn mutates(&self) -> Mutates // Never | Always | OnRepairOnly
fn repair_description(&self) -> Option<&'static str>
`RecoverableAdapter` forwards them — the registry only holds
`dyn JobHandler`, so a tenant's metadata is invisible otherwise, and
falling back to the defaults would report every recoverable job as
read-only, including the ones that delete files.
Three values rather than a boolean because a job can be read-only by
default and destructive under `?repair=true`; a boolean answers wrongly
for one of its two modes, and `false` on something that unlinks files is
the dangerous direction to be wrong in. `repair_description` returning
`Option` collapses "does it repair" and "what does repair do" into one
method: presence gates the toggle, content is the confirmation text —
which the frontend cannot invent, since correcting a counter and
deleting sidecars are not the same warning.
`OnRepairOnly` with no `repair_description` is rejected at registration:
it claims to mutate only under a flag it does not support.
All 17 registered jobs declare all three. The panel now renders the
description under each name, badges read-only jobs, confirms before a
plain run of a mutating one, and offers the repair variant off the
backend flag instead of the name list.
Descriptions are English in the trait, next to the behaviour: one in
`locales/*.json` rots invisibly the moment a job changes, and a
translator cannot know what `manifests_consistency` reconciles. i18n can
layer on later keyed by job name with these as the fallback.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
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.
No test passed repair=true, so verify_and_unlink, the
sidecar_delete_unverified finding and the directory removal had never
executed. That left the one destructive part of the migration as its
least-tested code: everything else is additive and recoverable, this
unlinks files after a readback check, and a defect costs bytes.
Four assertions, because "the files are gone" cannot by itself tell a
correct drain from a destructive one:
sidecars gone — the drain happened
rows still present — it deleted the COPY, not the record. Removing
the row would strand the blob in exactly the way
the bulk-reap bug just did: a live reference
with nothing behind it, which GC is then correct
to refuse forever.
unverified == 0 — every unlink passed its readback rather than
being skipped, which is the property that makes
deleting safe at all
directory absent — the signal step 10e gates on, and why remove_dir
is used: it refuses a non-empty directory, so
success proves emptiness rather than asserting it
Preconditions assert the sidecars exist first, or a no-op run would pass
all four by doing nothing. The directory check logs rather than fails,
since a concurrent render could legitimately repopulate it.
The run gave the decisive fact: `derived=1`. A content_derived_blobs row
still points at the leftover blob, so GC is CORRECT to keep it — the
leak is the row, not the bytes. purge_derived_blobs only runs when the
SOURCE is reaped, so the question is why that never happened.
So the dump now prints the source hash and what still holds it:
src_files, src_manifest, src_blob. If the source has a live file the
answer is "not deleted"; if it has none but a positive refcount, a
release was missed upstream; if it has no row at all, the source was
reaped WITHOUT purging, which would be a real ordering bug in reap_blob.
Also fixes the dump reporting only one of three blobs. `docker compose
exec -T` reads stdin, so it consumed the rest of the here-string feeding
the loop — the other two were never queried and vanished silently. The
same silent-truncation shape the diagnosis exists to expose, in the
diagnosis. `< /dev/null` closes it.
52c31a68 added a per-leftover refcount dump to storage_cleanup_check.sh
but referenced COMPOSE_FILE, which that script never defines — only
thumb_import_check.sh does. It would have run `docker compose -f ""`,
failed, and been swallowed by the `|| true` guarding the loop.
A silent no-op: the diagnosis would print nothing and the failure would
look exactly as uninformative as the one it was written to explain. The
same shape as the three bugs this suite has already caught — an error
dressed up as an unremarkable result — and I wrote it into the tool
meant to find them.
The `|| true` stays, so one unreadable blob cannot abort the loop before
the others report.
Three blobs survived the sweep. Five seconds of async-unlink polling did
not remove them, so they were never queued — GC had not judged them
collectible, and the loop had already exited.
It broke on the FIRST zero-reap pass. A single zero only says nothing
was collectible at that instant: releases cascade, since reaping a
source drops the references its derived and attached rows held and
`on_blob_deleted` does that from spawned tasks, so a pass can land in
the gap between "source reaped" and "dependents released" and report
zero with work outstanding. The import jobs added a level to that chain,
which is when it started biting. Now two consecutive zeros, with the
bound raised to match — one extra trigger over an empty store is
cheaper than a false pass reporting a clean disk.
The rest is diagnosis, because a list of paths cannot tell the three
causes apart and they need opposite fixes: a positive refcount means a
release was missed, an orphan means the reap predicate has a gap, and a
row without a manifest means the registry is inconsistent. Each leftover
now reports its manifest and blob refcounts plus how many files, derived
rows and attached rows point at it — so if this is a real leak rather
than the race, the next run names it instead of costing another full
pass through the suite.
storage_cleanup_check.sh asserted a sidecar exists on disk after
fetching a thumbnail. Correct while `.thumbnails/` was the durable
store; wrong since 10d2 removed that write. The check failed on exactly
the behaviour it was meant to confirm.
Inverted rather than deleted, because the inverse is the more useful
guard: a sidecar reappearing means a write path regressed to the legacy
shape, which would silently make `.thumbnails/` un-emptyable and strand
step 10e forever — its gate is the directory being gone, and a single
recreated file holds it open.
The HTTP 200 above already proves the thumbnail works; this now proves
it got there the new way.
Both of the helper's streams are silenced at the call site. It reports
absence loudly — red banner plus a `find` dump — because absence used to
be the failure; here it is the expected result, and leaving that visible
would cry wolf on every clean run.
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.
from_accept returns JPEG unless Accept contains image/webp, and no
thumbnail test sent the header — curl defaults to */*, which does not
match. So the whole suite ran on JPEG and the WebP path was never
exercised over HTTP, despite being what background generation writes and
what the derived tier was built around. The gap was invisible because
the JPEG results were all correct.
Three assertions, one property each.
Content-Type proves negotiation happened: thumbnail_content_type sniffs
the body with `infer` rather than echoing the request, so image/webp
cannot be right by accident — serve JPEG bytes down the WebP path and it
reads image/jpeg and fails.
Differing bytes prove they are genuinely two artifacts rather than one
served twice.
Differing ETags prove the validators are separate. `variant` has carried
the format only since 20261022000000; before that a JPEG request could
match the WebP row and be served the wrong codec, and a shared validator
is exactly how a cache would then hand either to either. A final
conditional request confirms each codec revalidates against its own.
Together these also cover the per-format variant keying that lets one
source hold both codecs — the prerequisite for JPEG clients ever leaving
the sidecar, and therefore for step 10e.
Note on placement: thumb_etag stays in step 4's capture block, beside
thumb_bytes. Every later request omits Accept and so negotiates JPEG, so
the validator must be the JPEG one — captured after the new block it
would describe a different codec than the bytes next to it, and the
copy assertions compare against both.
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.
Uploading without folder_id answered `500 Internal Error: folder_id is
required to determine file owner`. A missing required field is the
caller's error; as an internal_error it produced `error_type: Internal
Error`, which the SPA cannot distinguish from the server breaking — so a
malformed request looked like an outage.
Both sites become validation_error (ErrorKind::InvalidInput → 400), with
messages that say WHY the field is needed rather than restating that it
is: the destination folder determines the file's owner and drive.
The OpenAPI request body described it as "optional folder_id field",
which is how it came to be omitted — hit while writing
thumbnail_etag_content_keyed.hurl, where the upload was written from the
documented contract and 500'd. Now stated as required.
Regression test asserts the status AND that error_type is not "Internal
Error", since the contract the SPA switches on is error_type rather than
the message.
`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.
Nothing exercised these jobs. Their unit tests cover the directory walk
— which files each claims — but neither had ever executed a run.
The test environment always starts fresh, so there is no pre-migration
data to import. This creates it, and the reconstruction is EXACT rather
than an imitation: the on-disk layout did not change in this work. A
rendered thumbnail has always been written to {size}/{hash}.webp and an
uploaded preview to {size}/ext-{id}.jpg; the only new thing is the row.
So upload through the real API, then delete the row, and what remains on
disk is byte-for-byte what a pre-migration install has.
Deleting the row must also release the reference it held, or the
manufactured state would carry a reference no legacy install ever had
and storage_cleanup_check.sh would report a leak this script caused.
file_attached_blobs has an ON DELETE trigger for that;
content_derived_blobs does not — its Rust purge path releases explicitly
— so the strip decrements it directly.
Three assertions, in increasing order of what they catch:
1. Both rows come back, and the imported attached row carries the nil
uploader sentinel rather than a fabricated one.
2. A COPY inherits the imported preview. This is the user-visible
point and was impossible before the row existed: the ext- sidecar
is keyed by file_id, no copy path duplicates it, so the copy
silently fell back to a render.
3. Re-running imports nothing and changes no refcount. The likeliest
silent defect — store_attached_blob is ON CONFLICT DO UPDATE, so an
import that skipped its existence check would release and retake a
reference every run, invisible except as drift.
psql runs inside the compose container rather than depending on a host
binary, matching how spawn-db.sh probes readiness. Ordered before
storage_cleanup_check.sh, which deletes everything it needs.
Thumbnails were served `public, max-age=31536000, immutable`. Two
problems, and the first is a security one.
`public` on a Permission::Read gated resource lets any shared cache — a
corporate proxy, a CDN — store one user's thumbnail and serve it to
another. `Vary: Accept` was no defence: it does not vary on
Authorization. Now `private`.
`immutable` was a promise this URL cannot keep. It is keyed by file id,
and its bytes change when a preview is uploaded, when content is
replaced, or when an attachment is removed. `immutable` tells a client
not to revalidate at all during the freshness lifetime, so with a
one-year max-age a browser that fetched once would never see a new
preview — which also made the content-keyed ETag unobservable in
practice. A correct validator is worthless if nothing asks. Now
`no-cache`, which still stores the body and only requires revalidation,
answered by the ETag with a body-less 304.
The hurl tests could not have caught this: hurl always sends the
request, so If-None-Match was exercised and passed while a browser
obeying `immutable` never got that far. Same "correct on the wire, wrong
in practice" shape as the bugs before it, so the test now asserts the
directives themselves rather than only the 304 behaviour.
One definition, shared by the REST and NextCloud endpoints, which are
gated identically and must not drift. /_app/immutable is untouched:
those are hash-named static assets, genuinely content-addressed and
public, where the directive is honest.
Cost is a conditional request per thumbnail per page load. Recovering it
needs a content-addressed URL — where `immutable` would be true — but
that puts the hash in the URL of an authorized resource, so it stays
`private` regardless, and it touches the SPA and the file DTO. Separate
change.
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.
The disk check proves no BYTES are left. This proves no ROWS are, which
fails differently and worse: a stale storage.blobs row with nothing
behind it means a reference was never released, and dedup_gc will skip
it forever because its count never reaches zero. Silent, permanent, and
invisible to a check that only looks at the filesystem.
Zero is the right assertion, not "fewer than before". By this point the
suite has deleted its users, their drives and everything cascading
beneath, and the disk check has already insisted the blob store is
empty. A non-zero registry beside an empty disk is exactly the
divergence the consistency jobs report — caught here first because one
number is easier to read than a findings list.
Degrades to a warning if the endpoint is unavailable rather than
failing, so a build without the admin dedup surface still runs the rest.
The disk checks above prove nothing leaked. These prove the bookkeeping
behind them is honest: every refcount matches what the reference sources
hold, and no row points at bytes that are gone.
End of suite is the only place this is cheap. One database serves every
hurl file, so by here the counters have absorbed every upload, copy,
move, share, trash and purge the suite performed — across both copy
paths, the derived tier and the attached tier. Drift that no individual
test would notice, because each only inspects its own file, surfaces as a
mismatch.
Runs after the GC drain deliberately: mid-sweep state is legitimately
inconsistent — a manifest can sit at zero waiting for the next pass — so
checking earlier would report normal in-flight state as drift.
Zero findings is the assertion. These four tenants are read-only, so
anything they report is a real invariant violation rather than a repair
opportunity. A job missing from the build is skipped with a warning
instead of failing, so this does not break on a feature-gated build.
Unknown job names and unwrapped-vs-wrapped response shapes both degrade
to a visible warning rather than a silent pass: list_job_runs currently
returns a bare array, and the .runs/.items fallbacks exist so a future
wrapping does not quietly turn the whole check into a no-op.
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.
GET /api/files/{id} is the download route — it returned the PNG bytes,
so the jsonpath capture failed on a UTF-8 decode. /{id}/metadata is the
EXIF endpoint and carries no FileDto either. Listing the folder gives
the DTO, and since the folder holds exactly this one file, count == 1
also proves the WebDAV PUT overwrote in place rather than creating a
second file beside it.
Also drops an unused bytes capture and records why the body is not
asserted after the overwrite: the moka tier is keyed on file_id and
invalidated from the spawned task in on_file_updated, so a request
landing first sees the previous bytes under the new ETag. Asserting on
bytes would be a race.
The upload omitted folder_id, which the handler needs to resolve the
file's owner — it answers 500, not a root upload. Every other upload in
the suite passes it; this was the only one that did not, which is why
nothing caught it earlier.
The folder also gives the WebDAV overwrite a deterministic path
(/webdav/hurl-etag-src/<name>) instead of depending on where a
folder-less upload would have landed. Teardown now removes it and
purges it from trash, keeping the shared database clean for the files
that run after.
The thumbnail ETag was "thumb-{file_id}-{size}-{format}", sent with
Cache-Control: public, max-age=31536000, immutable. Replacing a file's
content preserves its id — file_upload_service rebuilds the entity with
parts.id and a new hash, then fires on_file_updated, which deletes and
regenerates the thumbnails — so the server produced a new thumbnail while
still advertising the old ETag. Because `immutable` tells a conforming
browser not to revalidate at all inside the freshness window, clients kept
rendering the previous image for up to a year, unfixably.
Keyed on the content hash the directive becomes honest: a thumbnail is a
pure function of (source bytes, size, format), so that triple identifies
the response. New content yields a new ETag.
The same change fixes the opposite direction. A copy, or any dedup twin,
had a different id and therefore a different ETag, so clients refetched
bytes they already held even though both are served from the same derived
blob. Now identical content agrees on an ETag and revalidates to 304
across files, users and copies.
Both thumbnail endpoints were affected: the REST handler and the
NextCloud preview handler.
Cost is one PK lookup ahead of the 304 decision, where the id-keyed
version needed none — paid for by no longer serving stale images. It is
partly recovered: both handlers already resolved the same hash further
down for the render path, and that second lookup is now gone, so the
cache-miss path is unchanged and only the 304 path pays. The resolved
hash is also handed to get_cached_thumbnail instead of None, saving the
service its own lookup.
No new disclosure: content_hash is already on FileDto and returned by
GET /api/files/{id}.
Tests: thumbnail_etag_content_keyed.hurl covers invalidation — overwrite
in place via WebDAV PUT, assert the ETag changed, assert a client holding
the stale one gets 200 rather than 304. derived_blob_copy.hurl gains the
sharing direction: a copy answers with the SAME ETag and revalidates to
304, which is the one externally observable consequence of content-keying
and was not previously testable.
The byte-identity assertions were documented as proving that a copy
shares the original's content_derived_blobs row. They prove no such
thing: rendering is deterministic in the source bytes and the variant,
so a copy that re-rendered from scratch returns identical bytes. The
copy is in fact a moka hit — that cache is keyed on
(source_hash, size, format), which the copy shares — so it never
reaches the derived tier here at all.
Nor is there an assertion that would fix it. Duplication is impossible
by construction: the PK is (source_hash, kind, variant), a copy carries
the same source_hash, and store_derived_blob is ON CONFLICT DO NOTHING.
The schema enforces the property, so no runtime behaviour can violate
it and there is nothing to catch.
Same limitation narrows step 11: it proves the SOURCE content survived
GC, not the derived blob — a reaped derived blob is re-rendered
transparently from the live source.
What the file does prove is unchanged and is the part that was broken:
both copy paths take a real blob reference (ref_count 1 -> 2 -> 3), and
purging the original does not destroy the copies. No assertions changed.
Step 8 of docs/plan/derived-blobs.md. "What follows a file on copy" was
written twice — the copy_file CTE and storage.copy_folder_tree — and had
already drifted: the tree path bumped storage.blobs only, missing
manifests, which was silent data loss on any multi-chunk file. Fixing it
meant writing the same logic a second time. Step 9 adds a file-keyed
satellite table, which would mean a third and fourth.
Two SQL functions:
storage.add_blob_references(TEXT[]) — the manifest-first reference
contract for SQL callers, returning hashes that matched no registry
row. Set-based so the tree path keeps its single-statement cost; a
per-row helper would have made a 10k-file copy 10k calls.
storage.copy_file_satellites(UUID[], UUID[]) — dead properties plus
the blob reference. The body is the copy-semantics declaration: what
is absent (comments, favorites, content-keyed derived rows) is listed
with its reason, so the taxonomy is executable rather than documented
elsewhere and drifting.
Both copy paths now call it. The single-file path becomes a real
transaction, which also fixes the reference being best-effort: a failed
add_reference used to log a warning and leave a copy holding no
reference at all — the exact shape that gets its content reaped. It
cannot be a CTE arm, because data-modifying CTEs share one snapshot and
the function must read the row the INSERT just wrote.
Verified against a scratch PG with all migrations applied: multi-chunk
manifest 1→2, single-chunk alias bumped at manifest level only (the
NOT EXISTS guard), chunks behind a manifest untouched, dead properties
duplicated, length mismatch rejected, repeats counted.
tests/api/derived_blob_copy.hurl covers it end-to-end and answers the
question the copy raises: content_derived_blobs is NOT copied. A copy
carries the same blob_hash, so it resolves the same derived row — the
test asserts byte-identical thumbnails from both copy paths, then
deletes the original, runs GC, and requires both copies to still serve.
That last step only passes if the references are real.
One dedup_gc pass cannot fully drain now that thumbnails are derived
blobs. Reaping a source releases the references its derived artifacts
hold (each content_derived_blobs row pins a manifest), and those
releases happen mid-sweep — the derived chunks are stamped orphaned as
the pass is already walking past them, because
remove_manifest_reference deliberately does not unlink, to avoid racing
a concurrent upload re-referencing the same chunk. They are collectible
only on the NEXT sweep, which is why the check saw 15 leftover blobs.
Loops until a pass reclaims nothing rather than hardcoding two. Two is
correct only while the derivation graph is one level deep — a thumbnail
is derived from a file, nothing is derived from a thumbnail. That is a
property of the data, not an invariant the code enforces, so a fixed
count would silently under-drain the day transcodes-of-thumbnails or
E2E-wrapped derivatives exist, and the failure would surface as a
confusing leftover-file assertion rather than the design change it is.
Bounded at 3 with a warning if it does not settle.
Sleeps between passes. The JobRegistry serialises runs of the same job,
so a back-to-back trigger risks rejection as already-running — which
returns 0 reaped and would exit the loop early, declaring success with
blobs still on disk. A false pass is worse than a slow one. It also
gives the previous pass's detached unlink tasks (spawned by
on_blob_deleted, awaited by nothing) time to land.
Deliberately NOT fixed in production code: derived chunks land inside
the 1-hour orphan grace, so a second immediate sweep would collect
nothing there and the next scheduled run picks them up. A fixpoint loop
in garbage_collect would be dead code outside force=true, which is only
this test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 'broken pipe' error under set -euo pipefail was masking the real
answer. Capture status + content-type + first-char in one curl, dump
first 200 bytes of body on failure so we can see what the server
actually served.
Edouard Vanbelle