The SQL repositories gated the name ILIKE condition on name.len() >= 3
*bytes*, so a 1-2 character search (e.g. "ab") silently returned an
arbitrary page of the caller's files instead of matches — while the
suggest dropdown (which never had the gate) still found them, making the
top-bar search feel broken. The gate existed because the pg_trgm GIN
index cannot accelerate sub-trigram patterns, but wrong-but-indexed is
never acceptable: caller/folder scoping still bounds the scanned set and
result pages are LIMIT-bound.
Add a shared name_filter_active() predicate next to like_escape() (any
non-blank query filters) and use it at all 7 gate sites — 4 in
file_blob_read_repository (condition/bind pairs kept in lockstep so bind
indices stay aligned), 3 match guards in folder_db_repository. The
Tantivy >= 2 gate on the content index is deliberately left alone: it
means "too-short tokens don't enter the full-text index", not a
correctness gate.
Co-Authored-By: Claude Code <noreply@anthropic.com>
Every thumbnail request paid an uncached storage.file_attached_blobs
point query before it could answer — including 304 revalidations and
RAM thumbnail hits, where the ETag path (thumbnail_content_id) probes
the row every time and tier 2b probes it again with the same key. A
photos grid revalidating 60 thumbnails per visit meant 60+ point
queries per browse, repeated on every visit.
find_attached_blob now reads through a process-local moka cache in
DedupService, keyed by the row's (file_id, kind, variant) PK, holding
positive and negative entries (most files have no attached preview, so
the negative side carries the win). Two rules keep it honest:
- DB faults are surfaced as Err and never cached — a transient outage
cannot freeze "no attached blob" into a negative entry (a read
failure is never proof that data is absent). The public signature is
unchanged; the SQL body moved to find_attached_blob_uncached.
- Writes invalidate eagerly: store_attached_blob and the Inserted arm
of store_attached_blob_if_absent on success, and deletions via
ThumbnailRefreshHook::on_file_deleted, which all three production
delete paths (single file, folder cascade, trash clear) fire after
the DELETE commits. The 60s TTL bounds only what the process cannot
see (bare SQL, copy_file_satellites races).
The Nextcloud preview endpoint rides the same lookup and benefits
identically. Five in-memory contract tests pin the cache behaviour,
including the fault-not-cached rule.
Co-Authored-By: Claude Code <noreply@anthropic.com>
The public landing page's inline media preview (added in 6ee26e46)
requests /api/s/{token}/file/{item_id}, but assert_file_in_share went
through resolve_folder_share, which hard-rejects non-folder shares —
so for a single-file share the video src got a 400 and the player
rendered empty: the preview box appeared but nothing would play.
The AuthZ gate now branches on item_type instead: a file share only
accepts file_id == share.item_id, a folder share still requires the
file to live in the shared subtree, and anything else is NotFound
(same shape as "file doesn't exist", preserving anti-enumeration).
Password/expiry checks still happen inside
get_shared_link_with_unlock, unchanged.
Also extend public_shares.hurl section 8b: the file-share token must
stream its own item (200 + inline disposition) and reject an outsider
file id with 404.
NOTE: fmt/clippy/api-test could not run on the authoring machine (no
Rust toolchain or Docker) — run `just check` + `just api-test` before
pushing.
Co-Authored-By: Claude Code <noreply@anthropic.com>
SO_REUSEPORT is Unix-only; the parameter is only read inside the
#[cfg(not(windows))] block, so -D warnings fails the build with an
unused-variable error on Windows hosts while Linux CI stays green.
Co-Authored-By: Claude Code <noreply@anthropic.com>
The sync sweep (and the EXDEV copy fallback) opened blob files with
File::open — a read-only handle — before calling sync_all. POSIX fsync
accepts read-only fds, so Linux never noticed, but Windows
FlushFileBuffers requires a GENERIC_WRITE handle and fails with
ACCESS_DENIED (os error 5) on every call. On Windows deployments the
strict sweep therefore failed every deferred sync, and the post-copy
fsync silently never happened.
Files now open via OpenOptions::write(true); the best-effort directory
fsyncs keep the read-only POSIX dirent idiom unchanged.
Co-Authored-By: Claude Code <noreply@anthropic.com>
The public share page only rendered media previews for FOLDER shares —
a single-file share got a bare icon + download button, even for images
and videos the browser can play natively.
Backend: resolve the shared file's mime_type + size at read time and
expose them on ShareDto (meta + password-verify endpoints, one shared
enrichment helper). Display-only enrichment: a failed file lookup
leaves the fields None instead of failing the response — the download
endpoint still surfaces the real error.
Frontend: the 'file' view now reuses the folder grid's lazyVideo
(poster-seek + retry) for video and imageRetry for images, with
Range-aware streaming already provided by /api/s/{token}/file/{id}.
Co-Authored-By: Claude Code <noreply@anthropic.com>
- normalize username into lowercase (this is already ASCII only)
- permit users to login with their username with insensitive case
- if a disabled account is reactivated and got a collision, it will normalize it too
- server will stop on collision (ex: 2 entries with `Alice` and `alice`)
in a such case admin can run:
```
oxicloud migrate lowercase-usernames --dry-run
```
then
```
oxicloud migrate lowercase-usernames
```
- do not increment ref_count if new attachement to a file with same data
- add audit log to help identifying other future issue in ref_count
- prevent race condition while attaching a blob
- server now provide it's config via /api/config (possibility to feature flag)
- client use /api/config to enable / disable some features
- capability to disable the message bus, somme OPS may not want this feature and
consume persistent connections from server (websocket):
OXICLOUD_MESSAGEBUS_ENABLE (true by default)
Ed's completed migration reported `copied: 0` beside
`scanned_count: 2522`. Both numbers were accurate; they were measuring
different things and neither said which.
`scanned_count` was cumulative because `checkpoint` had been persisting
it after every batch. `copied` / `skipped` / `failed` / `source_missing`
were plain locals initialised to zero at the top of the handler, written
to `stats` only via `merge_stats` — which is engine-only and fires on
`Completed`, a state a paused run never reaches. So every pause threw
them away and every resumed segment started counting from nothing.
## The fix has two halves, and only one is the obvious one
Restoring on resume is the obvious half: the four counters now seed from
`stats` exactly as `already_scanned` already did.
The half that actually matters is WHEN they are written. Restoring is
useless if nothing durable exists to restore from, so counters are
persisted per batch through a new handler-callable
`checkpoint_counters`, immediately after the cursor checkpoint.
`merge_stats` stays engine-only; the end-of-run summary write is
unchanged.
Two deliberate choices:
* **Absolute values, not deltas.** The merge is last-write-wins and the
handler owns the running total. Deltas would double-count on exactly
the replay path that produced 2522 scanned against 2022 rows.
* **A counter-write failure warns, it does not fail the run.** The
cursor is the correctness-critical write; these are reporting. Losing
a migration to a hiccuping stats merge is the wrong trade.
`scanned_count()` is now a default method over the new generic
`stat_u64(key)` rather than a second near-identical query.
## Not fixed, and not claimed to be
The 2522-vs-2022 overshoot itself. This makes it legible — cumulative
and per-segment values now both land on the row — but whether the final
segment re-walked rows it had already counted is a cursor question that
needs reproducing, not inferring. The counters should let it be observed
next time rather than reconstructed afterwards.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
From Ed's completed migration, which reported success while still
carrying the reason it had stopped hours earlier:
"status": "Completed",
"error_message": "target backend init: Transient Backend:
Cannot access bucket 'test-oxicloud': …"
The resume UPDATE flipped `status` to Running and refreshed
`last_progress_at` but left `error_message` alone, so a message
describing why the LAST attempt stopped survived every subsequent
segment and outlived the condition entirely. The run recovered; the row
still said otherwise.
Ed placed it exactly: the same stale-state shape as the read-only banner
that kept showing after its migration was over. State that describes a
past condition has to be cleared by whatever ends that condition, not
left for a later writer to overwrite by luck.
Cleared on resume rather than on completion, because resume is the point
the condition demonstrably no longer holds — and it also fixes the
intermediate reads, where a Running row would otherwise show an error
for work that is actively progressing.
Comment lives in Rust, not in the SQL string: the query text goes over
the wire on every execution and ends up in pg_stat_statements, where
prose is noise.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
From Ed's local→S3 outage run, which paused correctly and then said:
event="job.run" job=backend_migration outcome="ok"
... "paused":true,"retryable":true
This is the State-vs-Outcome distinction Ed drew earlier, in a channel
the earlier fix did not touch. The admin panel now separates the two;
the scheduler's own log line only ever carried the outcome, so a
migration frozen on an unreachable backend read as a clean run at INFO.
`JobOutcome` has just `Ok` and `Err`, and a pause is carried as `Ok`
with `paused: true` in `extra` — correct in itself: the handler did its
job and stopped cleanly at a checkpoint. The persisted shape is
unchanged for that reason. But projecting it to `outcome="ok"` tells an
operator the opposite of what they need to know, which is that nothing
will progress until the backend returns and someone resumes.
Paused runs now log at WARN with `outcome="paused"`, a `retryable`
field, and a message saying so. `grep 'outcome="ok"'` no longer matches
a blocked migration.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ed pulled the network mid-migration and got nothing: no log, no pause,
after more than two minutes. The cause is not the classification work
that preceded this — it is that there was no error to classify.
Pull a network on an ESTABLISHED TCP connection and there is no RST and
no ICMP. The peer simply stops answering and the socket read blocks
until the OS abandons retransmission, on the order of fifteen minutes.
For that whole window the job is neither running nor failed. Nothing
retries, because nothing failed. It looks exactly like a slow migration.
A refused connection is instant and does surface, which is what made
the earlier `127.0.0.1` test look reassuring. It exercised the one
network failure that cannot hang.
## Two layers, because one does not fit
`TimeoutBlobBackend` is innermost, below retry — a hang has to become an
error before any layer above can react to it. Bounds are per operation
class, because one number cannot fit both a HEAD and a 5 GB upload:
metadata 30s exists / size / delete / init / health / list
open 60s time to FIRST BYTE, not transfer duration
write off the whole transfer is inside the future, so any
bound here is also a maximum upload duration
Write is unbounded by default deliberately: guessing it wrong truncates
legitimate uploads, which is worse than the hang it would prevent. All
three are configurable (`OXICLOUD_STORAGE_TIMEOUT_*_MS`, 0 = unbounded).
The S3 client also gets what it could always have had. It was built from
a bare `config::Builder::new()`, which carries NO `TimeoutConfig` at
all — so `SdkError::TimeoutError`, an arm `s3_domain_error` already
handles, was unreachable. It now sets connect/read timeouts plus
stalled-stream protection, which measures throughput rather than
elapsed time and is therefore the correct instrument for a stream: it
bounds a stalled upload without capping how long a large one may take.
## Local is not the justification
Ed's correction, and it is right: a local path is reached through the
kernel, and the kernel owns that timeout. iSCSI gives up after
`replacement_timeout` (120s default) and returns an I/O error; NVMe-oF
and soft-mounted NFS behave the same. Those arrive as `io::Error` and
`local_io_error` already classifies them. Local passes through the
decorator only because a uniform chain beats a conditional one, and a
bound that never fires costs nothing.
The real asymmetry is Azure: its 0.21 client has no timeout knob short
of a custom transport, and the SDK migration is deferred. That is why
this lives in the chain rather than being configured per SDK.
Also fixes the log gap: the timeout warns with the wrapper, backend,
operation and bound, so a stalled layer is visible before the pause
rather than only afterwards.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit fixed get / get-range / stat but left `blob_exists`
returning `internal_error` on S3 and Azure, which undoes the point of
the exercise: `blob_exists` is the FIRST call `backend_migration` makes
against the source for every blob.
match self.source.blob_exists(hash).await { // migration, per blob
An unclassified error there is permanent, so a refused connection during
a migration takes the permanent branch — record a finding and move on —
which is the skip-and-advance behaviour the pause was added to prevent.
The classification has to hold at the probe, not only at the read that
follows it.
Both now classify before deciding: only a genuine 404 / `is_not_found`
answers "absent", everything else keeps its transient class. On S3 that
means classifying the `SdkError` by reference first, since
`into_service_error()` consumes it.
Local was already routed through `local_io_error` at its stat site.
Audited the rest of the S3 surface: initialize, put ×3, get, get-range,
delete, stat, list and exists all classify. The two remaining
`internal_error`s in `put_blob` read a *local* source file, so there is
no network class to preserve.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ed's point, and the most dangerous bug in the batch: NotFound is a
conclusion callers ACT on. Every read path in all three backends
returned it unconditionally.
// s3, azure, local — all of them
.map_err(|e| DomainError::new(ErrorKind::NotFound, …))
So a refused connection, a 503, an expired credential, a stale NFS
handle and an unmounted iSCSI target all reported "blob missing". Nine
sites: get / get-range / stat on each backend.
## Why it is disastrous rather than untidy
`backend_migration` probes its source before copying. A transient probe
error used to `continue` — skip the row, record NOTHING, and let the
cursor advance past it at the end of the batch. With `failed` still 0
the run reached `finish_completed` and FLIPPED THE POINTER to a target
missing every blob the outage covered. A migration reporting success
having silently dropped whatever was unreachable at the time.
That path now pauses when the probe error is transient, and records a
finding when it is permanent, so a run can no longer report clean while
having skipped rows.
## Local storage is not exempt
Ed again: a local backend is a PATH, and that path may be an iSCSI or
NVMe-oF LUN, an NFS mount, or a disk with a failing sector. It matters
MORE there than for a remote backend, because `RetryBlobBackend` is only
applied when the active backend is not Local — nothing below retries, so
the classification is the only thing between a flaky mount and a run
concluding the data is gone.
`local_io_error` maps the network-mount family (TimedOut,
HostUnreachable, NetworkDown, ConnectionReset, StaleNetworkFileHandle)
plus Interrupted and ResourceBusy to transient. PermissionDenied,
ReadOnlyFilesystem and StorageFull stay permanent because retrying
changes nothing without an operator, and InvalidData stays permanent
because corruption is a finding worth keeping. A bad sector arrives as
an uncategorised EIO and lands there too, which is right: the useful
outcome is a finding naming the blob, not a run that waits for a disk to
heal.
## Shape of the fix
Only a genuine absence is NotFound — `NoSuchKey` on S3 GET,
`is_not_found` on S3 HEAD, HTTP 404 on Azure, `ErrorKind::NotFound` on
local. Everything else goes through the classifier, so a 403 stays
permanent rather than being retried forever.
Tested at the local layer, which is where the mapping table is dense
enough to get wrong.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ed's diagnosis, and it is the root of three symptoms I had been patching
one at a time: a job has two independent statuses, and the panel was
collapsing them into one column.
* STATE — where the run is in its lifecycle: running, paused,
cancelled, completed, failed.
* OUTCOME — how the work turned out: ok, issues, notices, err.
They are orthogonal. A paused run has no outcome yet. A completed run's
outcome may still be "issues". Conflating them produced, in order:
1. a paused migration rendering as a green "ok" — the outcome was
genuinely ok, the STATE was Paused, and only the outcome was shown;
2. my first fix, which put "blocked" into the OUTCOME column — a
category error, encoding lifecycle into the result axis;
3. a cancelled job still reading "blocked", because that outcome was
cached in memory while the cancel had flipped the row in SQL.
The layout already had both columns. State just never rendered anything
but "running" or "—", so the status axis had no home and the information
leaked into Outcome.
Now:
* State renders `last_run_status`, sourced from the run ROW. Memory
cannot answer this — it is empty after a restart and stale after a
cancel, both of which the row gets right. The retryable reason, when
there is one, is the pill's tooltip.
* Outcome goes back to describing only the work: ok / issues /
notices / err. No lifecycle in it.
`JobSummary` gains `last_run_status`, and `last_run_at` falls back to
the row's `started_at` when memory has none — a restart left the column
reading "never" for a job whose last run was hours earlier.
"never" is now reserved for jobs that genuinely never ran. With a run
row present but no cached outcome the cell reads "—": the honest "no
outcome recorded", rather than a claim the run history immediately
contradicts.
The enrichment query generalises rather than multiplying — it already
fetched Paused rows for the Resume button, so it now takes the latest
row per job via `DISTINCT ON` and derives state, timestamp and paused
brief from it. Sound as "the current run" because the
`one_active_run_per_job` partial unique index permits one non-terminal
row per job and a resume reuses it, so a non-terminal row is always
newest.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`backend_migration` tolerated a failed copy by recording a
`migration_failed` finding and moving to the next blob. Correct for one
corrupt object — a single bad blob must not abort a migration of
millions — but wrong when the backend has simply gone away: every
remaining blob then fails, each records a `data_loss` finding, and the
run walks the whole space to reach a conclusion available in seconds.
**The cursor is what makes skipping unsafe.** It advances to the
batch's LAST hash, after the inner loop. So continuing past a transient
failure lets the batch finish and the cursor move BEYOND the blob that
failed, and nothing revisits it — the run ends carrying a `data_loss`
finding for a blob that was never damaged, only briefly unreachable.
Ed caught this reviewing a first version that tolerated N consecutive
transient failures before pausing: that variant skipped up to N blobs
per batch for exactly this reason. The threshold is gone.
A transient failure now pauses on the FIRST occurrence. The cursor is
still at the previous batch's end, so a resume re-walks the batch and
retries the blob; re-copying already-present blobs is free because the
walk short-circuits on them. Permanent failures keep the old
tolerate-and-continue, which is what it was built for — retrying them
would fail identically.
`migration_readonly` stays engaged across the pause, so Cancel remains
the way to release it.
Cost of pausing eagerly is small: `RetryBlobBackend` has already made 4
attempts (0 / 100 / 200 / 400 ms) before the error arrives here, so a
pause means the backend was unreachable for ~700 ms of trying, and
Resume is one click that continues from the cursor.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ed proposed the obvious end-to-end test — point an S3 entry at
127.0.0.1 with nothing listening, get a refused connection, expect a
transient error — and it would have failed, because `initialize()` was
the one SDK call still wrapped as a plain `internal_error`.
That is the FIRST call both jobs make, so it is what a wrong-endpoint
test actually hits: `backend_consistency` and `backend_migration` each
return `Failed` on init, and every classification added in the previous
commits sits downstream of a path the test never reaches.
Now `head_bucket` goes through `s3_domain_error` like the rest, and both
call sites route through `RunOutcome::from_domain_error`. A refused
connection or a 5xx pauses and can be resumed once the endpoint returns;
a wrong bucket or bad credentials is 4xx and stays terminal, which is
the distinction that makes pausing safe to offer at all.
No cursor at init — nothing has been scanned — so the pause resumes from
the start, which is correct rather than lossy.
Worth noting for `backend_migration`: target init runs BEFORE
`migration_readonly` is engaged, so pausing there holds no write freeze.
An operator can leave it paused indefinitely and resume when the target
comes back, with no read-only window.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Step 4 of docs/plan/jobs-handling-recoverable-error.md — the sharp edge
the plan flagged, and it was already a live trap independent of the
retry work.
`backend_migration` engages `migration_readonly`, which refuses writes
ACROSS THE WHOLE APPLICATION until cutover. Cancelling it cleared
nothing. The flag is persisted, so the state survived restarts — boot
even logs a warning about coming up read-only — and the only escape was
editing `admin_settings` by hand.
Two paths reach a cancel, and only one of them ran any handler code:
* a RUNNING row re-enters the handler, which now releases the gate at
its next cancel poll when the intent is terminal;
* a PAUSED row does NOT. `request_terminal_cancel` flips it straight
to Cancelled in SQL with no handler in the loop.
The second is the common case and the one that matters: a migration
paused by an outage, holding the freeze, cancelled by an operator
precisely to get writes back. Fixed in the cancel endpoint, which is the
only place that sees it.
Releasing on cancel is safe because cancel ENDS the run with no swap —
the source is still the active backend, so nothing is left to protect,
and a later retry starts fresh and rescans everything.
**Pause deliberately keeps the gate**, per Ed's call: Ops cancels to
release it. That is not conservatism for its own sake. The cursor is a
position in a hash-ordered walk and stays valid only while nothing
writes; release the gate on pause and a blob written afterwards whose
hash sorts BELOW the cursor is never visited, so the run completes,
flips the pointer, and reads for that hash 404 against a target that
never received it. Releasing on pause becomes safe only once resume
rescans from the start or a final catch-up pass runs under the freeze
before the swap — the plan's follow-up, not this commit.
Both release paths are best effort: a run that has already been
cancelled should not become a hard failure because a DB blip prevented
clearing a flag. The in-memory store happens regardless, so writes
resume in this process; a loud warning names the DB copy needing
attention.
The endpoint check is gated on the job name AND on the flag currently
being set, so it is a no-op for every other job — nothing else ever sets
it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
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>
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>
cjw
Dionisio Pozo