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
```
Records the design for issue #691 (make usernames case-insensitive):
silently lowercase on ingest, explicit `oxicloud migrate
lowercase-usernames [--dry-run]`, refuse-to-boot until DB is fully
lowercase. Includes the chunked-upload directory rename step and the
OIDC JIT lowercase fix surfaced during the design sweep.
Design deferred pieces (display_name split, WebDAV URL redesign) are
listed under "Not in scope" so the boundary is explicit.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- 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)
It still said "Status: not started" after the whole thing shipped and
was validated by hand against a real S3 endpoint in both directions.
Steps 1–4 marked DONE, §Testing marked DONE with the two places the
implementation departed from what the section anticipated:
* The fixture is an unreachable ADDRESS, not Azurite. Azurite's
deterministic 500 is a *failure*, and failures were never the hard
case — they surface and get classified. What hung was a peer that
never answers. Also records that the existing `s3_stub`
(`127.0.0.1:9999`) cannot serve this: nothing listens, so the
connection is refused instantly and a test built on it would pass
with no timeout configured anywhere.
* The bound is a polling budget, not a request duration.
`backend_migration` is detached — the trigger returns 202 in
milliseconds however long the backend hangs, so timing it proves
nothing. That mistake was made and caught in review.
The header also records the two things the DESIGN did not anticipate,
because they explain why the policy alone would not have been enough:
classification cannot see a call that never returns (no error to
classify), and `NotFound` was being returned for every read failure at
nine sites — including `blob_exists`, the migration's first probe of
the source, which made a transient outage look like an absent blob and
could flip the pointer to an incomplete target.
Remaining work left explicitly open: the online-migration shape, the
stacked-retry tuning, and the one unreproduced `scanned_count`
over-report.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two fixes from Ed's run.
## The assertion I called load-bearing was measuring nothing
`backend_migration` is a DETACHED job: the trigger spawns the handler
and returns 202 in milliseconds, carrying no outcome and no run_id.
I had modelled it on `admin_jobs.hurl`, where the jobs are synchronous
and the response IS the outcome.
So `duration < 120000` on the trigger would have passed against the
ORIGINAL unbounded behaviour — it timed the dispatch, not the
migration. The one assert the file existed for proved nothing.
The bound is now a polling budget: `/runs?limit=1` with `retry: 60`,
`retry-interval: 2000`. 120s, then hurl fails on the last assert.
Against a 15-minute hang the row sits in `Running` and the budget
exhausts, which is the failure this file is for. `run_id` comes from
`$[0].id` (runs are `ORDER BY started_at DESC`), since the 202 body
has none.
## A count assert on a registry, again
`storage_multi_entry.hurl` asserted `$.entries count == 3` and
`s3_blackhole` made it 4. The failure reads "expected 3, got 4",
naming neither the entry that appeared nor whether it belonged.
Replaced with per-name `contains`, which is what a registry wants:
membership asserted per item, so declaring a new entry does not break
an unrelated file. Positional asserts stay — entry ORDER is a separate
property and a real one, since the boot fallback picks `[0]` when no
active pointer exists.
Its comment also said "Two entries declared" while asserting three:
the drift a count invites, visible in the same three lines.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The regression test `docs/plan/jobs-handling-recoverable-error.md`
§Testing asks for: assert the run reaches Paused, that `error_message`
names the cause, and that it does so in bounded time rather than
hanging.
## The endpoint has to HANG, not refuse
`s3_stub` already existed and points at `127.0.0.1:9999`, where nothing
listens. That connection is REFUSED — ECONNREFUSED, immediately — and
that path was never broken. A test built on it would pass with no
timeout configured anywhere, which is worse than no test: it would read
as coverage of exactly the failure it cannot see.
So `s3_blackhole` points at `192.0.2.1`, TEST-NET-1 (RFC 5737),
reserved for documentation and guaranteed unrouted. A SYN goes
unanswered — no RST, no ICMP — which is the failure that used to hang
until the OS abandoned TCP retransmission ~15 minutes later, with the
job neither running nor failed the whole time.
Ed's suggestion, and it is the right fixture: a server that never
answers is reproducible in a way that unplugging a cable is not.
## The load-bearing assertion is `duration`
Every other assert in the file would also pass against the old hanging
behaviour, given fifteen minutes. `duration < 120000` is the only one
that fails if the bound is ever removed. The threshold is deliberately
loose — three orders of magnitude from the failure it guards, so a slow
runner cannot make it flaky.
## Why it is safe in the shared suite
The run fails at `target.initialize()`, which is BEFORE
`migration_readonly` is engaged, so this file cannot leave the server
read-only for whatever runs next. A mid-copy failure would have held
the freeze — that is why this shape was chosen.
Teardown is mandatory rather than tidy: `open_or_start` picks up the
latest non-terminal row, so a Paused row left behind would be RESUMED
by the next `backend_migration` trigger in the suite, silently
retargeting an unrelated test at the black hole. The file cancels its
own run and asserts the row reached Cancelled.
Placed second-to-last. It is the slowest file in the suite by design —
it waits out an unreachable endpoint to prove the wait is bounded — so
that cost lands after everything else has reported. Azurite stays last
for the reason its own comment gives.
Not yet executed: the suite tears down containers and Ed usually has a
run in flight.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
cjw
Dionisio Pozo