Step 3 of docs/plan/jobs-handling-recoverable-error.md, and it
deliberately does NOT add the retry loop the plan sketched. Reasoning
below.
`RunOutcome::from_domain_error(cursor, context, err)` routes a failed
operation to `PausedRetryable` when the error is transient and `Failed`
otherwise. Handlers call it instead of reaching for `Failed`, so an
outage stops a long scan at its cursor rather than discarding it —
`Failed` is terminal, and only `Paused` resumes.
Applied to `backend_consistency`'s enumeration failure first, because
that is the case with the most to lose: the job fails the whole run on
an enumeration error, so a brief 503 partway through a million-object
bucket used to throw away the entire audit.
## Why no bounded retry loop in the engine
The plan said "bounded exponential backoff, ~5 attempts" in
`run_or_resume`, and also warned "do not double-retry — the AWS SDK
already retries internally, so a second layer above it multiplies".
Checking before writing it, there are already TWO layers:
* the AWS SDK retries internally;
* `RetryBlobBackend` wraps every remote backend with exponential
backoff — 3 retries, 100 ms initial, ×2, 10 s cap, all tunable via
OXICLOUD_STORAGE_RETRY_*, and applied in di.rs for non-Local
backends.
A third layer multiplies rather than adds: one logical operation could
span SDK × decorator × engine attempts, turning a brief outage into
minutes of held `migration_readonly` — the precise failure this plan
exists to stop.
Retrying here would also re-run a SCAN, not an operation. The retrying
belongs where it already is, per request; what was genuinely missing is
the conversion of an exhausted-retry failure into a resumable pause with
a reason, which is what this commit adds. If the attempt budget needs
tuning, `OXICLOUD_STORAGE_RETRY_MAX_RETRIES` is the knob, and it applies
to every backend call rather than only to jobs.
## Tests
`transient_failure_pauses_with_a_reason_and_keeps_the_cursor` asserts
the three things that matter: status Paused, cursor preserved,
`error_message` naming the cause. `permanent_failure_still_fails_terminally`
is the control — without it the classification could be inert and
everything would simply pause, which would look like success.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Step 2 of docs/plan/jobs-handling-recoverable-error.md. A handler could
say `Completed`, `Paused` or `Failed`, so a transient backend failure was
flattened into `Failed` before the engine saw it — "the provider is
down" and "this data is wrong" were indistinguishable, and `Failed` is
terminal, so an outage threw away a partially-complete migration.
`PausedRetryable { cursor, reason }` lands as `Paused` in the row, so
resume is unchanged. What differs is `error_message`:
| outcome | meaning | resumes? |
|-------------------|----------------------------------|--------------|
| Failed | the data or request is wrong | no, terminal |
| Paused | an operator asked it to stop | yes |
| PausedRetryable | the environment failed | yes, + why |
Without the reason a paused run is an unexplained one — and a paused
`backend_migration` still holds `migration_readonly`, refusing writes
application-wide, so "why is this app read-only" has to be answerable
from the row.
`mark_paused_retryable` is a separate store method rather than an extra
argument on `mark_paused`: only one of them writes `error_message`, and
a `reason: Option<&str>` parameter would let a caller produce a Paused
row carrying an error message and no error — the exact state this exists
to distinguish from.
Reported as `JobOutcome::ok`, not `err`. The run did not fail; it
stopped and can be resumed. A red job in the panel that a Resume click
fixes reads as a bug rather than as a decision waiting to be made. The
`extra` carries `retryable: true` and the reason so the panel can say
which kind of pause it was. Audited too, since a run that stopped on an
outage is an operational event someone has to act on.
## Also: Azure now classifies its errors
The previous commit said Azure could wait for the official-SDK
migration. That was wrong — `azure_core::error::ErrorKind::HttpResponse`
carries the status on the archived 0.21, so `azure_domain_error` works
today. It matters because Azure is the backend this whole plan was
written for.
Applied at five sites including the 256-shard enumeration walk, where
`backend_consistency` fails the entire run on an error, so a throttle
partway through should be retryable rather than discarding the sweep.
Per Ed's call on the ambiguous case: a deterministic 500 — Azurite
answering the CRC64 ranged GET, every time — classifies as transient
because nothing at this layer can tell it from a passing one. Retry as
if transient, let the bounded cap convert the difference into a Paused
run, and let Ops decide to resume or cancel.
Not yet wired: the engine's bounded backoff (step 3). Note for that
work — backoff already exists in the AWS SDK internally AND in
`RetryBlobBackend` (100 ms, ×2, 10 s cap, 3 retries). A third naive
layer would multiply, so the plan's "do not double-retry" needs
measuring before adding one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Step 1 of docs/plan/jobs-handling-recoverable-error.md, and the blocker
for the rest of it: the engine cannot retry-then-pause until it can tell
"the provider is down" from "this data is wrong". Both arrived as
`ErrorKind::InternalError`, so the distinction survived only inside a
formatted message.
`RetryBlobBackend` was reading that message. Literally:
let msg = err.to_string().to_lowercase();
msg.contains("timeout") || msg.contains("503") || msg.contains("reset by peer")
Fragile in a specific way — an SDK reformatting its `Display` turns
retrying off with nothing failing to say so — and blind to any status
code that never made it into the text.
Adds `ErrorKind::TransientBackend` and `DomainError::is_transient()`.
One predicate, so the retry decorator and the job engine cannot classify
the same failure differently. `Timeout` counts (transient by
construction); everything else must say so explicitly. The default is
"not retryable" because that fails visibly, whereas retrying a permanent
fault burns attempts and — once the engine wires this up — holds
`migration_readonly` while it does.
A kind rather than a `transient: bool` field: 21 struct-literal sites
construct `DomainError` directly and would all have needed touching for
a change that is conceptually about classification. The plan allowed
either.
`s3_domain_error` does the classification where the status is still in
hand. Transient: 5xx, 429, and the SlowDown / RequestTimeout /
ThrottlingException codes that arrive as 400 (status alone is not
enough), plus dispatch-level I/O and timeouts. Permanent: other 4xx —
credentials, missing bucket, malformed request — and construction
failures. `ResponseError` counts as transient since truncation on the
wire is the usual cause and the attempt cap bounds being wrong.
Applied at the five S3 sites that wrap an SDK error, including
`ListObjectsV2` — `backend_consistency` fails the whole run on an
enumeration error, so a throttle midway through a large bucket should be
retryable rather than discarding the sweep.
The exhaustive `ErrorKind` match in `interfaces/errors.rs` forced the
HTTP decision, which is the right friction: 503, not 500. The request
was fine and may succeed shortly, which is what a caller needs to decide
whether to retry and what a proxy keys off to avoid caching the failure.
The substring matcher stays for now, behind the typed check, with the
deletion condition written down: it goes when every backend wrapping a
remote SDK error classifies at the point of wrapping. Removing it before
then would silently reduce retrying on the unconverted backends, which
is the worse direction. Azure is the one left, and it is queued for the
official-SDK migration anyway.
Not yet wired: `RunOutcome::PausedRetryable` (step 2) and the engine's
bounded backoff (step 3). This commit only makes the distinction
representable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`04807464` made deep-mode cancellation responsive but paused at the
BATCH-START cursor, which throws away everything done in the current
batch. Ed caught the sharp edge while testing pause: the checkpoint only
lands after a batch completes, so a run paused 27 s into its FIRST 63 s
batch had `cursor_hex: ""` and would resume from scratch. Later batches
lose 500 blob reads, about a minute against remote S3.
Now the pause carries `settled` — the highest hash whose pair was fully
handled. That is safe because the merge-join advances both sides in
ascending hash order: at any point in the loop, everything at or below
`settled` has had its findings recorded and, under `?deep=true`, its
bytes re-hashed. So resume re-does one pair, not the whole batch.
`settled` only advances after an arm finishes with its item, never on
entry, which is what keeps that invariant true.
Also checkpoints explicitly before returning `Paused`, with
`delta_count = 0` since `scanned_count` is already credited per batch.
The engine writes the cursor on the Paused row anyway; persisting it
here means a restart racing that write still resumes from the right
place rather than the previous batch.
Strictly fewer duplicate findings on resume, too. Page-level
`unknown_backend_file` notices are emitted before the join, so any
resume re-emits those for the re-walked range — a shorter range is
simply less of it. That duplication is pre-existing and orthogonal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two problems a real S3 run exposed, both about a completed run being
unable to answer questions about itself.
## Which storage did this verify?
A finished run recorded `deep`, `verified`, `total_rows` — but not its
target. Findings carry `"backend"`, and a clean run has none, so a green
audit says nothing about what it audited. After switching the active
backend there is no way to tell what a previous run covered.
That is not hypothetical: a 1.5 s local sweep was read as an S3 audit by
both of us for several exchanges, and the run JSON could not settle it.
What settled it was the ABSENCE of a `storage` param, inferred by hand.
Now the outcome carries `backend` (the type), `storage_entry` (the
entry name) and `scoped` (whether `?storage=` was given). The entry name
is read from `admin_settings` at run start rather than snapshotted at
boot, because a migration cutover rewrites it while the process lives —
a cached copy would name the pre-cutover entry, which is the same
staleness trap the `uncached()` unwrap avoids by resolving through
`current()`. Best-effort: it is a label, and failing an audit over one
would be the wrong trade. `ActiveEntry::Unset` stays unlabelled rather
than guessing at the boot fallback.
## Cancel was bounded by a batch, and a batch got 60,000x slower
`BATCH_SIZE`'s comment claimed 500 "keeps the cancel-poll cadence
sub-second (each batch = one backend list + one DB probe + Rust
set-difference)". True when written. Deep mode then moved into this
tenant and added 500 full blob reads per batch: measured at 155 ms each
against OVH S3, so ~63 s per batch. The status poll ran only between
batches, so Pause and Cancel appeared ignored for a minute — on exactly
the run an operator most wants to stop, and one that scales to hours on
a real corpus.
Cancellation is now polled inside the verify loop every
`DEEP_CANCEL_POLL_EVERY` (16) blobs. A poll is one indexed DB read
(~0.1 ms) against a 155 ms remote read, so the cost is under 1% there
and a couple of percent even on a local backend where a verify is
~0.8 ms. `BATCH_SIZE` goes back to being purely about I/O batching, and
its comment now says so.
Pausing mid-batch is safe: the cursor still points at the last completed
batch, so a resume re-verifies this batch's handful of blobs rather than
skipping them. Re-reading a few is the right direction for a check whose
whole purpose is not missing anything.
Measurements quoted throughout are from a live run: 2022 chunks (231
files), 314 s against remote S3 versus 1.567 s local, verified 2022 in
both.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`backend_consistency ?deep=true` re-reads every chunk and re-hashes it
to catch silent bit-rot, and records `blob_corrupted` (severity
`data_loss`) naming `backend.backend_type()`. It was reading through the
live backend — which for a remote backend includes `CachedBlobBackend`,
whose `get_blob_stream` returns the local cached file and never touches
the remote on a hit.
So the attribution was false in both directions: rot on S3 hidden by a
good cached copy, and rot in the cache reported against a healthy S3 —
the second sending an operator to the wrong layer entirely.
Surfaced by a real run: 2022 chunks, 321 ms shallow, 1.5 s deep. That is
0.74 ms per chunk for a full read plus BLAKE3, sequential, over S3 —
impossible, and explained by every chunk being cache-warm. A genuine
uncached sweep is tens of seconds.
Adds `BlobStorageBackend::uncached()`, defaulting to `None`.
`CachedBlobBackend` returns its inner; `Retry` and `Swappable` forward
so the unwrap reaches the cache through them. `Swappable` resolves via
`current()` rather than capturing a handle, because it sits OUTSIDE the
cache — a DI-time snapshot would keep pointing at pre-cutover storage
and audit the backend a migration just moved away from.
Only the cache is peeled. The cache stores plaintext and the content
hash is over plaintext, so unwrapping past the encryption decorator
would hand back ciphertext and fail every blob it checked.
**No change to normal reads.** `uncached()` is called in exactly one
place, and the unwrapped handle is used at exactly one call site
(`verify_bytes`). Enumeration, every other job, and every request path
still go through the cached stack.
`?storage=<entry>` was already correct — `build_entry_backend` has no
cache decorator — so this only fixes the live-backend path, which is the
one that was silently fast.
Also reports `verified` in the run extras, on every run including zero.
A deep run that verified nothing and one that verified everything were
otherwise indistinguishable in the outcome, which is what made a 1.5 s
"deep" sweep look plausible in the first place. Same lesson as
`orphans_covered` on the Azure fallback: a check that cannot report its
own coverage will eventually be believed when it should not be.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`JobRunArgs` was a fixed struct — `force`, `deep`, `storage`, `repair` —
and six places hardcoded that same list: the engine's persist/restore,
the trigger endpoint's query type, the OXICLOUD_STARTUP_JOBS parser, the
frontend API wrapper, the panel's checkboxes, and `StartupTrigger` on
the wire.
Two costs. Adding a parameter meant editing all six, and forgetting one
dropped it silently — most damagingly in persist/restore, where a
resumed run lost it and a `?repair=true` migration came back as
discovery-only after a restart. And the panel offered the same knobs on
every job: only two jobs read `deep`, six read `repair`, so most of
those controls did nothing with no way to tell which.
Now `JobHandler::parameters()` returns `&'static [JobParam]` — name,
type (boolean/string/number), default, and the job's own description of
what it does. `JobRunArgs` holds a map keyed by those names.
Everything reads the declaration:
* `run_or_resume` iterates it to persist and restore, replacing
`const FLAGS` plus a `storage` special case. `storage` stops being
special — it was the one Option<String> among three bools.
* `dispatch` normalises every run against it, which is what makes "a
handler sees its declared parameters with their declared defaults"
true rather than usual. The periodic tick passes an empty
`JobRunArgs::default()`, so a `default: true` parameter would
otherwise read false on every scheduled run.
* The trigger endpoint takes free-form query params and rejects
undeclared ones with a 400 naming the real set, instead of ignoring
them.
* OXICLOUD_STARTUP_JOBS keeps raw pairs (config is parsed before the
registry exists) and validates at dispatch, where the error can name
the job's actual parameters. Still a boot panic, same as an unknown
job name — a typo'd `?repare=true` must not leave a migration
importing forever in discovery mode.
* `JobSummary.parameters` carries it to the panel, whose `supportsDeep`
was a hardcoded name allowlist (`consistency_batch ||
backend_consistency`). A job gaining a deep mode needed a frontend
release; one losing it left a button that silently did nothing. The
menu now renders from the declaration, so a newly-declared boolean
appears with no frontend change.
Three consistency tenants were hand-rolling persist-on-fresh /
restore-on-resume for their own flag, under the same `params` key the
engine already used. Deleted — they read `args.get_bool(…)` now.
Fresh runs also filter to the declaration. `consistency_batch` forwards
its args verbatim to sub-jobs, so a tenant's `params` row could grow
`deep` with no deep mode, and the run-detail view would claim a mode the
job never had.
Two things found while wiring it, both worth knowing:
`RecoverableAdapter` bridges the two traits, and `parameters` has to be
forwarded there or the registry sees `&[]`. Both traits have defaults,
so omitting it compiled cleanly — and the trigger endpoint then rejected
`?repair=true` on the very jobs that declare it, with
OXICLOUD_STARTUP_JOBS panicking at boot. Now covered by
`adapter_forwards_job_metadata_from_inner_handler`.
`TriggerJobQuery` was briefly a newtype over the map. `serde_urlencoded`
cannot deserialize a newtype struct at the top level, so axum's `Query`
rejected EVERY trigger with a 400 — even one with no query string —
before the handler ran. It reads exactly like the new validation
rejecting something, which sent the first diagnosis to the wrong layer.
Now covered by `trigger_query_extracts_from_every_url_shape`.
Wire names are a compatibility surface: `params` rows are keyed by them
and the panel switches on them, so a rename breaks existing run history
the same way renaming a `Mutates` variant does. The JSON shape is pinned
in `snapshot_carries_job_metadata`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
Edouard Vanbelle