Replace plain `==` on lock tokens with `subtle::ConstantTimeEq` at
every token-comparison site on the WebDAV surface. Closes a
reported timing side-channel (2026-09-05) in `evaluate_if_header`
where an authenticated attacker could theoretically recover another
user's active lock token via response-latency measurements on the
`If:` header state-token comparison.
Practical exploitability is marginal — the signal is tens-of-ns
buried under ms-scale network jitter, ~5×10⁸ samples needed per
token to average through the noise vs a default lock lifetime of
60 s to 1 h — but the fix is a five-line change with zero
measurable perf cost (`subtle` is already transitive via
sqlx-postgres → digest, so no new binary weight), and adopting
constant-time compare on any token that gates access matches the
hygiene rule the rest of the codebase already follows on password
and session paths.
Sites fixed:
* `evaluate_if_header` — first-pass state-token scan and
second-pass condition eval in `webdav_handler.rs`.
* `WebdavLockService::refresh` — `!= token` mismatch check.
* `WebdavLockService::release` — `== token` guard on the
by_path invalidation branch.
The two `WebdavLockService` sites are already gated by
`self.by_token.get(token)?` — the attacker cannot reach the
comparison without already presenting a valid token, so their
timing surface is nil in practice. Kept constant-time anyway for
callsite consistency.
Sweep confirmed no other secret-adjacent `==` in production code:
password verification goes through Argon2's `verify_password`,
session/CSRF/DPoP jti tokens are hashmap-gated, and blob-hash
equality compares two server-side values with no attacker-
controlled operand.
Reported-by: Abdurazzoqov Javohir <abdurazzoqovjavohir700-dev@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
6dc045ea and 13a2f205 both say the bulk-delete residue "belongs to the
manifest-level refcount recompute (matrix row 7, still a gap)". **That is
wrong.** `ManifestsConsistencyCheck` exists and reconciles
`chunk_manifests.ref_count` against the same registry dedup_gc reaps
from; it is wired in `di.rs`. I took the claim from this table without
checking the tree, and then repeated it twice.
The table is what was stale, so fix it there:
* Row 7 — now `refcount_mismatch (manifests_consistency)`, ✓ at manifest
level, matching row 6's chunk-level entry.
* Row 8 — the predicate is registry-driven and no longer mentions
`ref_count` at all.
The blocker section is kept rather than deleted, because its reasoning
is why the predicate has its current shape, with a note on how it
actually resolved. The plan predicted 7 and 8 were coupled and had to be
fixed together, which was right — but it assumed the recompute would
make the counter safe to trust. The resolution inverted that: the reap
predicate stopped trusting the counter, which demotes drift from data
loss to a space leak the recompute then reports. Strictly better, since
it does not depend on a job having run recently.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GC phase 2 already had the right shape — `ref_count <= 0 AND NOT
EXISTS(manifest lists it) AND NOT EXISTS(file points at it)` — so unlike
phase 1 before 6dc045ea, a stale counter could only delay collection
there, never delete live bytes. What it did not have is any connection
to `BlobReferenceRegistry`: the two cross-checks named
`storage.chunk_manifests` and `storage.files` literally.
That is correct today and one source away from not being. Both
`content_derived_blobs` and `file_attached_blobs` return None at
RefLevel::Chunk, so the registry's chunk union is exactly manifests +
legacy files. The moment anything contributes at that level — a legacy
whole-file derived blob, or file_versions when versioning lands — phase
2 misses it and reaps referenced bytes. That is precisely the failure
the registry was built to prevent, and precisely what the phase 1
comment warns about while phase 2 sat unfixed.
## Why this is additive, not a swap
`no_reference_predicate` is assembled from fragments designed for
COUNTING, and FilesReferenceSource's chunk-level fragment deliberately
excludes files whose blob_hash has a manifest — otherwise a single-chunk
blob, whose file hash and lone chunk hash are the same BLAKE3, would be
counted at both levels. Correct for a recompute; too narrow for a reap
guard.
Concretely: a `storage.blobs` row keyed by a MULTI-chunk file's hash is
not a member of its own manifest's chunk_hashes, and such rows exist
transiently while `rechunk` migrates a legacy blob. Replacing the
hardcoded guards with the registry predicate would have satisfied
"unreferenced" for that row while a live storage.files row still pointed
at it — reaping it mid-migration. So the guards stay and the registry
predicate is ANDed on top. Adding a conjunct can only spare more rows,
never reap more, so this cannot regress; what it buys is that a future
chunk-level source is honoured automatically.
## Also: EXISTS instead of COUNT in the hot path
ChunksReferenceSource had no `ref_exists_sql` override, so the trait
default wrapped its counting fragment as `(SELECT COUNT(*) …) > 0`. That
now runs per candidate row inside the reap guard, and a
heavily-deduplicated chunk is exactly where counting every referrer is
most expensive and least necessary. FilesReferenceSource already carried
this override for the same reason; ChunksReferenceSource now does too.
Semantically identical, so no golden-test drift beyond the shape.
## Tests
`blob_reap_statement_is_stable` pins the assembled statement, and
`empty_registry_refuses_to_build_blob_reap_statement` mirrors the
manifest builder's loud failure on a wiring bug.
`a_new_chunk_level_source_reaches_the_blob_reap_statement` is the one
that earns its keep: since no shipped source contributes at chunk level,
a golden test alone would not notice the registry conjunct being dropped.
It registers a synthetic source and asserts the fragment appears.
Verified 921 passed / 0 failed on a clean database, and again on a second
consecutive run against the same one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`manifest_reap_sql` matched on `ref_count <= 0 OR <unreferenced>`, so the
counter alone licensed a delete. A reference that was never taken did not
merely report a wrong number — it made live content collectible, and the
registry that knew the row was referenced was never consulted, because
the first arm had already matched. `gc_spares_a_manifest_with_a_live_referrer`
(c9fc7dc6) demonstrated it against a real database.
The predicate is now `WHERE <no registered source references it>`.
`ref_count` does not appear in it at all.
Nothing is lost by dropping the arm. Its stated purpose was the
single-file delete path, where `cleanup_if_orphaned` decrements the
counter — but that path deletes the `storage.files` row too, which makes
the manifest unreferenced anyway. And it costs nothing: under `OR`,
Postgres had to evaluate the EXISTS union for every row whose
`ref_count` was above zero, which on a healthy install is nearly all of
them, so the expensive half was already running unconditionally.
What does change is the other direction. A counter stuck HIGH with no
referrers — the residue of bulk paths, where the trigger only touches
storage.blobs — is no longer reaped by the counter arm. It is still
reaped, because the registry says unreferenced;
gc_reaps_an_unreferenced_manifest_despite_a_high_refcount pins that, and
it is the test that proves this change did not trade one failure mode
for the other. Correcting such counters belongs to the manifest-level
refcount recompute (docs/plan/derived-blobs.md, matrix row 7), not to
the thing that deletes data.
`manifest_reap_statement_is_stable` is updated and now also asserts the
statement contains no `ref_count` at all, so a future edit cannot
quietly hand the counter its authority back.
## Test isolation, found the hard way
The new suite broke `garbage_collect_honours_grace_window_and_references`
— but only in the full run, and the failure pointed at that test rather
than at mine. Two distinct causes, both mine:
* `garbage_collect_force()` bypasses the CHUNK grace window for the whole
shared database, reaping sibling tests' just-uploaded orphans. Phase 1
has no time filter, so plain `garbage_collect()` proves the same thing
without the collateral damage.
* `GC_TEST_SERIALIZER` already existed for exactly this hazard, private
to `delta_upload_integration_tests`. Hoisted to module scope, with a
note that any test calling `garbage_collect*` must take it.
Attribution was worth the effort: restoring the `OR` did NOT fix that
test, which is what ruled out the product change and pointed at the
tests. Verified 918 passed / 0 failed on a clean database, and again on
a second consecutive run — the residue check that matters now that GC no
longer silently cleans up after a failed run by deleting referenced
manifests.
Pre-existing and left alone: `assert_eq!` with a literal bool in
delta_upload_integration_tests, warned by clippy only under
`--cfg integration_tests`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`manifest_reap_sql` matches on
WHERE m.ref_count <= 0
OR <no registered source references it>
An OR, so either signal alone deletes. Both arms have a reason — the
single-file delete path decrements the counter via `cleanup_if_orphaned`,
while bulk paths (user cascade, empty_trash) only fire the
`storage.blobs` trigger and leave it untouched, so the registry arm is
what collects those.
The consequence is that `ref_count` is authoritative on its own. Code
that fails to take a reference does not merely report a wrong number, it
makes live content collectible — and `FilesReferenceSource`, which knows
the truth, is never consulted because the first arm already matched.
`count_references` is implemented on all four sources and has no callers
at all; this is the gate it was written for.
Not hypothetical. `storage.copy_folder_tree` used to bump refcounts with
`UPDATE storage.blobs … WHERE hash = blob_hash`, which matches nothing
for a CDC file, whose `blob_hash` names a manifest rather than a chunk.
Copy a folder, delete the original, and the copy's bytes were reaped.
That bug is fixed — both copy paths go through
`storage.add_blob_references` — but the property that made it
destructive is unchanged, and there are now two implementations of the
reference contract (`storage.add_blob_references` in SQL,
`DedupService::add_reference` in Rust) that must agree forever.
Two tests, to be read as a pair:
gc_reaps_a_manifest_on_zero_refcount_alone passes — documents the
hazard, and fails loudly if the predicate is ever tightened, which
is the signal to delete it.
gc_spares_a_manifest_with_a_live_referrer FAILS — asserts the
contract worth having. Verified failing against a real database,
not inferred from reading the SQL.
The second is `#[ignore]`d only so a known-failing assertion does not
turn CI red while the fix is written; run it with
`cargo test --workspace --tests gc_spares -- --ignored`. Remove the
attribute in the commit that requires both signals.
That fix pairs with the manifest-level refcount recompute
(docs/plan/derived-blobs.md, coverage matrix row 7, still a gap): under
AND, a counter stuck high with no referrers stops being reaped by GC and
needs the recompute to correct it instead — which is where that case
belongs.
Fixture is deliberately multi-chunk and asserts so: a single-chunk blob
has `file_hash == chunk_hash`, the aliasing case the reference contract
carries a `NOT EXISTS` guard for, and testing it here would silently
exercise the easy path if CDC parameters change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Original correlated-subquery form was O(files × manifests) and
O(blobs × manifests) — hit statement_timeout on a large production
customer's DB and hard-failed app boot (migration rolls back → sqlx
marks failed → next start also fails; only recovery was bumping the
role-level timeout manually before restart).
Rewrite:
* SET LOCAL statement_timeout = 0 (tx-scoped, auto-reset at COMMIT)
— lifts the safety net for THIS migration only, so operators
with restrictive session defaults can complete the one-time
repair without intervention.
* Both UPDATEs replaced with WITH ... UPDATE ... FROM CTE + LEFT
JOIN patterns — single scans per source table, linear total work.
* unnest(chunk_hashes) replaces b.hash = ANY(...) — cost is
O(Σ chunk-array lengths), not O(blobs × manifests). No GIN
index needed.
Measured on sandbox with 303 rows of drift: 570 ms in the original
form. New form on 200 rows drift, cache-warm: 15 ms. Second run on
clean data: 10 ms no-op — idempotency preserved.
Content semantics unchanged — same auditor formulas, same idempotence
guarantee, same content-safety guarantees; only algorithmic
complexity + statement_timeout scope changed.
Adds an Azurite service and a scenario that audits the Azure backend
through `?storage=azurite`. It is the only coverage of that code path in
the tree: `AzureBlobBackend` has unit tests for its name parser and
ordering, but nothing else speaks the protocol, and a paid account is
not an option for CI. Azurite implements the real Blob REST API, so this
exercises SharedKey signing, prefix/marker paging, and the 256-way shard
walk with its termination.
## Harness
`docker-compose.test.yml` gains an azurite service on 10000 (tmpfs, so
it dies with the stack). `spawn-db.sh` provisions the container itself,
because `AzureBlobBackend::initialize` verifies rather than creates —
signed by hand with curl + openssl rather than pulling a ~700 MB `az`
image for one PUT. Two traps are commented there: the account key is
base64 but HMAC wants raw bytes, and the canonicalized resource repeats
the account name (`/{acc}/{acc}/{container}`) because the emulator puts
in the path what real Azure puts in the host. Getting that wrong yields
403, not a hint.
The `azurite` entry is declared in `server.env` but never activated, so
the suite's active backend stays local and only this file reaches Azure.
## What it asserts, and what it cannot
A failure surfaces as `ok: false`, because an enumeration error now
fails the run rather than degrading to a per-row probe.
It deliberately asserts no finding count. The container starts empty and
the job's grace window is an hour, so a freshly-uploaded blob is skipped
in both directions by design — an audit here can only report zero, and
"zero findings" would pass whether enumeration worked or returned
nothing. The one positive assert, `scanned_count != 0`, therefore sits
on the local control, which does hold blobs; `scanned_count` accumulates
via `checkpoint`, which the empty-page early return skips.
## No cutover, deliberately
Putting real bytes in the container means `backend_migration
?storage=azurite`, which hangs on the first blob: `head_check` issues a
~40-byte ranged GET, `azure_core` 0.21 attaches
`x-ms-range-get-content-crc64` to anything under 4 MiB, Azurite 500s,
and the deterministic error is retried forever while
`migration_readonly` refuses writes app-wide. The full chain and the
rejected workaround are in the file header. The scenario is still
ordered last in `run.sh` — it is the only one needing a second service,
and the cutover comes back there once the official SDK lands.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`AzureBlobBackend` inherited the trait's `operation_not_supported`
default for `list_blob_hashes`, so every `backend_consistency` run on
Azure fell back to a per-row probe. That fallback walks `storage.blobs`
asking "are these bytes there", which structurally cannot find orphans:
bytes no row claims are invisible to anything starting from the
database, because you need a hash to ask about one and discovering
unknown hashes IS enumeration. Azure had half the coverage of local and
S3, in the direction that wastes space.
## Enumeration
The obstacle was the cursor contract. The caller advances ONE cursor
across both sides of the merge-join, feeding the same value to the
backend and to `WHERE hash > $1`, so the cursor IS a blob hash. S3
satisfies that with `StartAfter`. Azure has no equivalent on this SDK:
REST 2023-05-03 added `startFrom`, but `azure_storage_blobs` 0.21 never
sends it — `ListBlobs` exposes only prefix, delimiter, max_results and
an opaque marker that cannot be derived from a hash.
Resume rides on `prefix` instead. Names are `{hash[0..2]}/{hash}.blob`,
which partitions the container into 256 shards that are themselves in
hash order, so walking 00/…ff/ yields exactly the global order the
merge-join needs and a cursor names the shard to restart in.
Re-listing on resume is bounded by shard width rather than by the whole
container — the cost a client-side skip over a flat listing would pay on
every page. `marker` pages within one call and never escapes as the
cursor, the same treatment the S3 impl gives its continuation token.
One asymmetry against S3, deliberate: constraining to `{2-hex}/` means
foreign files outside that shape never reach `unknowns`. Safe in the
direction that matters — an orphan is a blob we wrote and stopped
referencing, so it always has the canonical name — and it buys O(N)
enumeration instead of O(N²/limit).
`hash_from_blob_name` mirrors S3's parser, shard-equals-prefix check
included: without it a mis-sharded name would round-trip to a
`blob_name` we never wrote, reporting a live blob no read path can find.
Tested for round-trip, for nine non-canonical shapes, and for the
ordering premise the merge-join rests on.
## Removing the fallback
With Azure enumerating, nothing shipped answers
`operation_not_supported`. The fallback's other stated justification —
mid-migration — never applied: it named a `MigrationBlobBackend` that
does not exist, and `SwappableBlobBackend::list_blob_hashes` forwards to
whatever is currently active, as do the Encrypted, Cached and Retry
wrappers.
What still reached it was a transient failure (auth blip, throttle,
network) relabelled as a capability limit, on a run that then read as
clean while having silently lost orphan coverage. So it was not merely
dead, it produced the wrong outcome — the only one it could. It also had
zero test coverage across 227 lines.
Now any `Err` from `list_blob_hashes` fails the run. That is louder than
an anomaly on a green run, which was the fallback's own goal. The trait
default still returns `operation_not_supported`, so a genuinely
unenumerable backend would fail every run — detectable, not silent, and
the point at which to bring the fallback back with tests.
## Also here
A doc note on `get_blob_range_stream` explaining why the Azurite
migration hang is not worked around: `azure_core` 0.21's
`Range::as_headers` attaches `x-ms-range-get-content-crc64` to any range
under 4 MiB with no opt-out, Azurite 500s on it, and `azure_core`
retries a deterministic error forever. The reachable path is
`backend_migration` → `EncryptedBlobBackend::head_check` →
`get_blob_range_stream(hash, 0, HEADER_SIZE)` — a pre-write probe on the
TARGET, so it fires on the first blob while `migration_readonly` refuses
writes app-wide. Working around it would trade production read
amplification for emulator support; the fix is the official SDK, where
`range_get_content_crc64` is an explicit field.
`RUSTSEC-2026-0275` is ignored on the same reasoning — `azure_core` 0.21
logs the `authorization` header at debug, the advisory's "upgrade to
>=0.22.0" names a version that does not exist, and the real remedy is
that same migration. Reachable only via an explicit
`RUST_LOG=…,azure_core=debug`; the entry says not to run that against a
real account.
`docs/plan/jobs-handling-recoverable-error.md` covers the other half —
a bounded retry should have turned that hang into a Paused run with a
reason, whatever the SDK does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Step 12. The two tables existed with COMMENT ON text, but nothing
explained the pair together — and the relationship is the part that
matters: they hold the same kind of artifact under two different keys,
and the keying difference is a security boundary.
Content keying shares one derivation across identical bytes, which is
free and correct for something the server derived. Apply it to
user-supplied bytes and uploading a file whose content matches someone
else's lets you replace the preview they see. A single table with a
`kind` column cannot express that: the key has to be one thing or the
other, and either choice is wrong for half the rows. The split is the
enforcement, which is why the two import jobs each refuse the other's
filenames rather than one job handling both trees.
Covers structure, negative rows and what may not become one, the NULL
trap (comparison against NULL silently excludes negative rows —
correct for refcounts, wrong for dangling checks, fatal for
enumeration; all three have been hit), why `variant` carries the format
on one table and not the other, lifecycle and which consistency job
covers which failure, worked examples, and a decision rule for adding a
third artifact type.
Records `content_type`-as-key as a rejected alternative: reasonable
until negative rows made the column nullable, and PostgreSQL does not
allow a nullable column in a primary key. Worth writing down because a
later feature retroactively eliminated an option that would have looked
sound at the time.
Named for the two things rather than "satellite tables" — that is
internal shorthand nobody would search for, while `derived` and
`attached` are the words the schema and jobs already use. Mentioned once
in the intro so the code's collective noun still resolves.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The security boundary is a shape before it is a rule: DERIVED hangs off
content, ATTACHED hangs off the file, and two arrows starting from
different places carries the argument faster than the paragraph
explaining it.
Sketched inline rather than left as "add a diagram", so whoever writes
the page inherits the structure — including the parts a diagram is
uniquely good at showing: that both tables point into the same artifact
space (hence why one cannot be folded into the other with a kind
column), and that DERIVED.blob_hash is optional where ATTACHED.blob_hash
is not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tables exist and carry COMMENT ON text, but nothing explains the
pair together, and the relationship is the part that matters: they hold
the same kind of artifact under two different keys, and the keying
difference IS the security boundary. Content keying shares one
derivation across identical content, which is exactly what must not
happen for user-supplied bytes — one user's uploaded preview would be
served for every file whose content matches. A single table with a kind
column would not prevent that; the split does.
Today that reasoning lives only in scattered prose across this plan and
two job doc-comments, so someone adding a third artifact type has
nothing to read.
Scopes the page: column-by-column structure including the parts that
mislead (nullable blob_hash meaning a negative verdict, uploaded_by
NOT NULL with no FK), worked examples that make the rule checkable,
lifecycle and which consistency job covers which failure, and the
NULL-handling trap that has already caused two bugs — comparison
against NULL silently excludes negative rows, which is correct for
refcounts and wrong for dangling checks.
Lands in docs/architecture/ beside backend-storage.md, which documents
the blob layer underneath.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The plan still said "Scoped 2026-08-27, not started" for work that is
complete and validated against S3 — the first thing a reviewer reads,
describing the PR as unwritten.
Records the two pieces that were not in the original scoping: the memory
cache moving to content keying (it was still file-keyed, so identical
content held two RAM entries), and the engine binding a run to the flags
it started with. Also that `.transcoded/` legitimately persists where
external mounts exist, since hash-less callers have no content identity
— absence is only expected elsewhere.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The durable tier has been content-keyed since it was introduced —
`content_derived_blobs(source_hash, kind, variant)` — but the moka cache
in front of it was still `{file_id}:{ext}`, so the layer closest to the
request used the wrong axis while the layer behind it used the right
one. That was legacy shape, and I had defended it in a comment as
"deliberate: per-request-path and short-lived", which was a
rationalisation rather than a reason. Ed asked why, and there is no why.
Transcoding is a pure function of the source bytes. Under file keying,
two files with identical content held two RAM entries for identical
bytes, and the second file was a guaranteed miss that fell through to a
DB lookup plus a blob read to fetch what was already in memory under
another key.
Now keyed by content hash when the caller has one, by file id only when
it does not — the same `content` / `external` split `ThumbnailCacheKey`
already makes, and for the same reason: hash-less callers (external
mounts) have no content identity to key on. Prefixed `c:` / `f:` so the
namespaces stay disjoint; a hash and a UUID cannot collide in practice,
but "in practice" is how a file ends up served another file's bytes.
`invalidate` now clears only the file-keyed entry. Dropping content
entries there would be wrong, not merely wasteful: one file's content
changing says nothing about the other files sharing the old bytes, and
evicting theirs would make one user's edit cost everyone else a
re-transcode. Content entries need no eviction — new content is a new
hash, so the old key is never consulted again.
transcode_cache.hurl updated to match, and its header corrected: the
second file is now a RAM hit rather than a derived-tier read, so that
scenario can no longer isolate the durable tier. It says so, and points
at satellites_consistency and a restart as what covers it instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`run_or_resume` now records `JobRunArgs` in `params` on a Fresh open and
restores them on Resume, passing the restored args to the handler rather
than whatever the resuming caller supplied.
Two problems, and the engine is the only place both are guaranteed.
**A resumed run must not change mode.** Handlers read `args` on every
call, so a paused `?repair=true` import resumed by a plain trigger
silently continued as import-only: the deletion half never finished and
nothing reported it. `?deep=true` had the same hole — a paused bit-rot
scan resumed shallow while still presenting as the run that started
deep. `blobs_consistency` and `manifests_consistency` had hand-rolled
this for their own two flags; the three import jobs had not, and fixing
it per-handler means every future job remembering.
**The run row should say what it did.** For a destructive job, "did this
run delete anything?" is answerable only from `params`, which is what an
operator reads afterwards. Before this, a repair run and a discovery run
were indistinguishable in the history.
Deliberately not overridable on resume: adding `?repair=true` to a
resume would apply it to the remaining entries only, producing a run
that half-deleted. Cancel and start fresh is the honest way to change
your mind.
A missing key reads as false/None, so a run paused before this existed
resumes under-acting rather than deleting under a flag nobody gave it.
Failure to record the flags fails the run instead of guessing — acting
under unrecorded flags is the one thing worth refusing for jobs that
delete.
The flag list is hardcoded here. Letting each job declare its own
parameters — name, type, default — is the better shape and is its own
change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It was still registered on a 24h tick while the thumbnail imports moved
to on-demand. The same reasoning applies and I missed it: the boot run
in repair mode is the migration, nothing writes to that tree any more
so the tail cannot grow afterwards, and a tick could not finish the job
regardless because ticks never pass `repair`. Once drained it was a
`read_dir` returning nothing, daily, forever.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All three import jobs only checkpointed on a full batch, so the
remainder after the last one was never counted. A run shorter than
BATCH_SIZE never checkpointed at all: `scanned_count` stayed 0 against
a known `total_rows`, and the admin progress bar sat at zero for the
whole run and finished there.
Seen on a transcode_import run over 20 entries — 13 imported, 5
negatives, 2 already present, progress 0/20 throughout. The thumbnail
imports had it too, just less visibly: a 105-file run reported
`scanned_count: 100`, losing the tail rather than all of it.
Cursor-wise the final checkpoint is a no-op — the walk is finished, so
nothing resumes from it — but the scanned delta is what the progress
display reads, and it has to include the last partial batch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fire-and-forget raced its own purpose. A second request for the SAME
content arriving before the spawned write landed found no row, re-ran
the full decode + encode, and stored the identical blob again. Keying
derivations by content exists so identical content is derived once — a
write that has not landed yet cannot deliver that, and the window is
milliseconds wide exactly when it matters most, a page loading many
images at once.
Caught by transcode_cache.hurl, which asserts a second distinct file
with identical bytes does not re-transcode: `transcodes: 2` where 1 was
expected, `disk_hits: 0` where the derived tier should have answered.
It had been passing on timing luck.
The cost of awaiting is bounded. This path has just spent a full decode
and re-encode, so one blob write beside it is marginal, and it only
runs on a genuine miss — every subsequent request for that content is
served from the row.
The negative verdict was already awaited, which is why only the
positive half of the scenario failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Covers the job's surface: that it is registered, declares the metadata
the admin panel switches on (`mutates: always`, recoverable, a repair
description), and that a run against a drained tree completes cleanly
with zeroed counters.
It deliberately does NOT cover the re-keying, which is the part that
matters most. That needs `.transcoded/webp/` entries on disk before the
run, and nothing reachable over HTTP can create them — since the write
path moved to the derived tier, only hash-less callers still write
there, and hurl cannot place files in the server's storage directory.
The migration is validated by a snapshot restore instead, the way the
thumbnail one was; the file says so rather than implying coverage it
does not have.
The empty-tree assertions still earn their place. A drained tree is
what every run after the first sees, so it is the overwhelming
majority of this job's lifetime, and "does nothing, quietly" is a real
property: the thumbnail teardown warned `could not be removed / No
such file or directory` on every boot after its migration finished —
warning about success forever — and that was caught by eye, not by a
test. Counters are asserted as exact zeros, since finding work in a
directory the API cannot populate is the shape a re-keying bug would
take.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes the pattern the thumbnail migration established, for
`.transcoded/`.
`initialize` no longer creates the tree. Creating it at boot is exactly
what kept `.thumbnails/` alive across restarts — the import removed it,
the next boot put it back, and the absence the read path gates on was
unreachable by construction. The write path already calls
`create_dir_all` on the parent before writing, so eager creation
achieved nothing except defeating the drain.
It now probes instead: one `stat`, cached for the process lifetime, and
the local-cache reads short-circuit on a relaxed atomic load when the
tree is gone. Fails open, so a service built without `initialize`
behaves as before.
One difference from the thumbnail tiers, and it is not a stalled
migration: callers with no content hash — external mounts — cannot use
the content-keyed tier at all, so they still read and write here. On an
install without such mounts the directory drains once and stays gone;
on one with them it persists, correctly.
`transcode_import?repair=true` joins the startup defaults on the same
terms as the thumbnail imports, and with the weakest safety argument
needed of the three: a transcode is a pure function of its source, so
anything deleted in error is recomputed on the next request. The
`default_startup_jobs` test failed on the change rather than being
updated silently, which is what it is for.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The twin of thumb_derived_import, with the difference that shapes the
whole job: the legacy tree is keyed by FILE (`{file_id}.webp`) while the
destination is keyed by CONTENT. Thumbnail sidecars were already named
by blob hash, so importing them was a move; every entry here has to be
resolved through storage.files first.
That re-keying is the point rather than bookkeeping. A sandbox with five
.skip markers had three of them naming the same image, so the file-keyed
tree stored one verdict three times. After the import it is one row, and
any future upload of those bytes inherits it instead of paying for the
decision again.
Both artifact kinds are claimed by one walk: `{id}.webp` becomes a
derived Blob, `{id}.webp.skip` becomes a negative row. They share a
source file and a cursor, so splitting them into two passes would be two
chances for the pair to disagree about what had been handled. `.skip` is
matched BEFORE `.webp` — the shorter suffix matches a marker too, and
getting that backwards would read a zero-byte file and store it as the
transcode of its source, then serve it to clients. There is a test.
Entries whose file is gone cannot be re-keyed at all, so they are
reported and, under repair, deleted: unimportable by definition, and a
run that keeps rediscovering them never reports zero, so the gate for
removing the directory never opens.
Deletion reuses verify_and_unlink, so a cached transcode is removed only
after its stored replacement reads back byte-identical. Directory removal
follows the same rule as .thumbnails/ — delete, and only rename aside if
a non-cache file is in the way.
The batch checkpoint is a shared helper rather than inline at both exits
of the loop body. The first draft duplicated it and dropped the future on
the skip path without awaiting: the entry counted toward the batch, the
cursor never advanced, and a resumed run would have rewalked everything
it had already handled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Writing the hurl scenario surfaced a gap: a transcode that comes out
larger than the original runs a full decode + encode and increments no
counter at all. `transcodes` is bumped only on the success path, beside
`bytes_saved`, so the most expensive failure mode was invisible — a
multi-megapixel image decoded and re-encoded on every request, for
every file sharing that content, producing nothing.
That is precisely the cost the persisted negative verdict exists to
stop paying, and it could not be measured before or after. `not_beneficial`
counts it, kept separate from `transcodes` because conflating "work
done" with "work that paid off" would hide exactly what an operator
needs to see.
It is also what lets the hurl scenario assert the negative half: the
first fetch increments it, the second — a distinct file with identical
content — leaves it untouched, which is the negative row being read
rather than the verdict recomputed.
Assertions are exact equality against captured values throughout, no
`>` or `<`. A "greater than" would pass if a counter moved for the
wrong reason; equality against the prior reading catches any transcode
from any source, including one this scenario did not intend to cause.
Also fixes two URLs the first runs caught: file download is
`GET /api/files/{id}`, not `/content`, and the trash listing is
`/api/trash/resources`. And the duplicate uploads go to a second
folder — re-uploading the same filename into the same folder returns
the EXISTING file id, which would have made both halves of every
"two files, one content" pair the same row and left the scenario
asserting nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
Step 7 of docs/plan/derived-blobs.md, write path first — the plan is
explicit that fixing it before the import means transcode_import only
has to handle history, not a moving target.
ImageTranscodeService now reads and writes storage.content_derived_blobs
under kind='transcode', keyed by the BLAKE3 of the SOURCE content. The
hash is threaded in from file_retrieval_service, which already holds it
as dto.content_hash; hashing here would be a BLAKE3 over the whole file
on every request. Callers without one (external mounts) keep the local
cache untouched, which is what the service did before this tier existed.
Negative verdicts become rows rather than zero-byte .skip files. A
transcode that came out larger is deterministic in the content, so it is
worth remembering; the row survives moka eviction, a restart, and the
deletion of .transcoded/, none of which the marker does. Only that
verdict is persisted — a timeout or a read error returns Err and is
recorded nowhere, because a momentary failure written here would mark a
perfectly transcodable image hopeless with nothing to retry it.
Representation is a NULL blob_hash, per the plan: a sentinel hash would
stop blob_hash naming a real Blob and every consumer would need to learn
the exception. A CHECK keeps blob_hash and content_type NULL together —
a type without bytes describes nothing, bytes without a type cannot be
served.
Two consumers had to be corrected for NULLs first, both of which would
have broken on the first negative row ever written:
* satellites_consistency reported them as derived_dangling_blob at
data_loss severity. SQL comparison against NULL is NULL, so EXISTS was
false and a row correctly pointing at nothing read as an artifact that
had gone missing.
* blob_reference_sources::list_referenced_blobs decodes blob_hash into
String, so the first NULL would have failed the decode and taken the
whole enumeration down. It would also have been wrong if it decoded —
a negative row holds no reference, which is why the counting forms
(WHERE blob_hash = <hash>) already exclude it for free.
lookup_derived returns a three-way answer because Option collapses the
two cases a caller deciding whether to spend a decode most needs apart:
never attempted, versus attempted and known not worth it.
DedupService is attached after construction via a OnceLock. DI builds
the transcode service ~240 lines before DedupService exists, and the
retrieval path that needs it is wired earlier still, so a constructor
argument would mean reordering more than this is worth.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every startup job logged two info lines saying the same thing: the
scheduler engine's `job.run` (outcome + timing, which every dispatch
has always produced) and my `job.startup_completed` right after it.
Reading the boot log, that looks like the job ran twice.
Demoted to debug. The engine's line is the one that matters — logging
uniformly is the reason startup jobs go through `registry.trigger`
rather than calling handlers directly — and the `job.startup_trigger`
audit line before it already records that the startup path was the
caller, along with the flags it used.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Edouard Vanbelle