The Star History chart was broken because GitHub stargazer API restrictions disabled the previous service. Point the chart at a working alternative so the README graph renders again.
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>
A DPoP proof carries a server-issued nonce. With none cached the server
answers `401 use_dpop_nonce`, the client harvests the nonce and retries.
`signAndFetch` already absorbed that, so it was invisible — but it did
it PER REQUEST, with no coordination.
The Service Worker always cold-starts without a nonce. Both mechanisms
that pre-seed the page are unreachable from worker scope: workers have
no `sessionStorage`, and `seedNonceFromCookie` early-returns on
`typeof document === 'undefined'`. The SW also skips requests that
already carry a `DPoP` header, so it never observes the page's
responses and cannot harvest from them either. Browsers terminate idle
workers after ~30s, so this happens routinely, not once.
Uncoordinated, every request issued in that window discovers the nonce
independently: N parallel requests → N challenges → 2N requests. That is
precisely the photo grid, and serving `<img src>` is the SW's main job —
those requests cannot sign themselves, which is why the worker exists.
Each wasted challenge also costs the server a full ECDSA P-256 verify,
because `verify_proof` runs before the nonce check.
Now the first request through owns the discovery and the rest await it,
so N challenges collapse to 1. The wait is capped (5s) and released in a
`finally`: the SW is on the critical path for every thumbnail, so a hung
discovery must degrade to the old behaviour rather than stall the grid
behind a promise that never settles.
Measured on a 763-line e2e server log: 115 `dpop.nonce_challenged`
events across 97 logins.
Scope, deliberately: this does NOT remove the one challenge per worker
lifetime. Doing that needs the nonce persisted where a worker can read
it — IndexedDB already holds the keypair — and it can never replace the
challenge path anyway, since a persisted nonce can be stale. Left out
until the audit line shows it is worth it; the numbers above are now
legible enough to tell.
Not a correctness fix. Nothing was broken and no test failed over this.
The argument is waste, plus signal: 115 challenges per run is noise that
would bury a real one.
`hasNonce()` is exported for the gate — deliberately "will the next
proof carry a nonce", not "is it still valid", since only the server
knows the latter and the challenge path already handles it. Its tests
assert it agrees with what `buildDpopProof` actually emits, because a
wrong answer either reinstates the stampede or stalls every request
behind a bootstrap that is not happening.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An e2e run emitted 81 × 429 in 763 log lines. The env already set
LOGIN/REGISTER/REFRESH to 36000/hour, and that changed nothing, because
those three are the only rate limiters with env vars — and they are the
wrong ones. They key on the client IP and guard the unauthenticated
front door. The limiters that fired key on the CALLER ID.
The log distinguishes them: all 81 landed on target `http::api`, never
`http::api::auth`, where login/register/refresh live.
The likely culprit is `user_profile_rate_limiter`, 60 lookups/min/caller,
guarding the visibility query behind GET /api/users/{id}. The whole
suite runs as a single `admin`, so every test shares one bucket; admin
views resolve an owner name per row and the run creates 34 users, so a
minute of tests clears 60 easily. Nothing failed, because the SPA
degrades to an unresolved name — which is exactly the problem, since
that noise would hide a real rate-limit regression.
Adds OXICLOUD_RATE_LIMIT_USER_PROFILE_MAX / _WINDOW_SECS and
OXICLOUD_RATE_LIMIT_DELTA_UPLOAD_MAX / _WINDOW_SECS, following the
existing three exactly. Defaults are the literals they replaced (60/60
and 240/60), so an operator who sets nothing sees no change; a unit test
pins that, because the failure is silent in both directions — too low
and real users get 429s on listings, too high and the `access_grants`
query loses the guard that stops an attacker exhausting it with random
UUIDs.
`tests/common/server.env` (shared by the e2e AND hurl suites) sets both
to a 1-hour budget, matching the posture already used for the other
three rather than a raised per-minute rate that would still burst-trip.
The docs now state the IP-vs-caller split, since that is what decides
which knob to reach for — and note that several actors sharing one
identity (CI, a bot, a kiosk) share one caller bucket.
Left alone: the four narrower env files (OIDC, webdav-drive-root) keep
their existing MAX=3600 with default windows. No evidence they trip the
per-caller limits, and adding config on speculation is how these files
drift.
Not fixed here: rate-limit rejections emit NO audit line, which is why
the attribution above reads "likely" rather than "confirmed" — nothing
in the log names the limiter. AGENTS.md requires one for every
rejection; that is a separate change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `None` arm of the nonce match returns a challenge — the client sent
a valid proof but carried no nonce, so it gets 401 + `DPoP-Nonce` and
retries. It was the only one of the three challenge paths that logged
nothing.
The result was an unexplainable line in the access log:
WARN http::api: client_error status=401 latency_ms=0 …
with no audit line saying why, and no sign of the successful retry —
`main.rs` defaults the access log to `http=warn`, so the 200 that
follows is never printed. It came up as "any clue why I have 401?" while
reading an e2e run, which is the cost of a silent rejection.
AGENTS.md is explicit that every rejection emits a structured audit line
before returning the error; this path simply missed it.
The line pays off immediately: it names `htu`, and the first run with it
showed the caller was `GET /api/admin/plugins/{id}/logs/stream` — the
admin log tail, an `EventSource`, whose proof is minted by the service
worker rather than by `apiFetch`. `service-worker.ts` documents exactly
this: the nonce cache is per-scope, so the page module and the SW EACH
pay one round-trip challenge and then catch up from `DPoP-Nonce`
response headers. The SW absorbs the 401 and retries, so `EventSource`
never sees it. Hence "once per client SCOPE" rather than per session —
and again whenever the browser terminates and restarts the worker, which
is normal and is why one run shows several.
Deliberately NOT `dpop.verify_failed`. The proof verified cleanly —
nothing failed — and folding a routine bootstrap into the event
operators watch for attacks would bury real signal. `nonce_stale` above
keeps that event name because it is long-standing and aggregators key
off it; this path is new, so it gets the accurate one:
`dpop.nonce_challenged` / `reason = "nonce_missing"`. No new counter —
`nonce_challenge_response` already counts every challenge centrally, so
adding one here would double-count.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`apiAdminCreateUser` intermittently failed with
`401 {"error":"DPoP nonce required","error_type":"DpopVerificationFailed"}`,
most visibly in admin.spec.ts's pagination test.
Not a nonce-rotation race — the nonce pool keeps a 3-minute overlap
window precisely so in-flight requests survive rotation. It is a
service-worker-control race.
`browserFetch` issues a raw page-context `fetch`, and the DPoP proof is
attached by the Service Worker intercepting it — the helper's own doc
comment says so. The SPA's proof-and-retry logic lives in `client.ts`'s
`dpopFetch`, which this helper deliberately bypasses. So when the SW is
not yet controlling the page, the request goes out unsigned, the
middleware sees a bound session with no proof (`dpop.rs`, the
`expected_jkt` match), and answers with a nonce challenge. Nothing
retries it: the SW that would have signed it is what is missing, and
`dpopFetch` was never in the path.
The window is real on every fresh browser context. `service-worker.ts`
does `skipWaiting()` + `clients.claim()`, which is correct, but claiming
is asynchronous — the first navigation loads uncontrolled, then
install → activate → claim. `waitForLoadState('networkidle')` says
nothing about SW control, so a helper called a few lines after
`apiLogin` can land inside it. Slower CI widens it, which is why this
showed up there and not locally.
The fix waits inside the same `page.evaluate` as the fetch, so it costs
one property check once the page is controlled and needs no per-page
bookkeeping. It is bounded at 10s: if the SW never claims, the request
goes out as before and the resulting 401 stays the clear signal it is
today rather than becoming an unexplained Playwright timeout.
Worth stating because it is easy to get wrong: **`ready` is not
`controlling`.** `navigator.serviceWorker.ready` resolves once a
registration is active, while `controller` stays null until that worker
has claimed THIS page. Awaiting only `ready` looks correct and still
flakes.
This is a test bug, not a product one. Real paths go through `apiFetch`,
which signs in page JS; the SW is the safety net for requests that
bypass it (`<img src>`, downloads). This helper is the only caller
relying on the SW as its primary signer.
Not verified by running the suite: a flake reproduces on CI timing, so
one green local run would prove nothing. The mechanism is confirmed from
the code path — SW-signed proof, no fallback retry, asynchronous claim.
Co-Authored-By: Claude Opus 5 (1M context) <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>
Dionisio Pozo