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>
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 pointed a broken S3 entry at 127.0.0.1 with nothing listening. The
classification worked end to end — the run paused with
`target backend init: Transient Backend: … ConnectionRefused` — but the
job row showed a green **ok** pill and the reason was only visible after
expanding it.
`PausedRetryable` reports `outcome: 'ok'` on the wire, and that is
correct: the run did not fail, and a Resume continues it. But rendering
it as plain "ok" hides the one thing worth acting on. A paused
`backend_migration` is still holding `migration_readonly` and refusing
writes across the whole application, presented as a healthy job.
The row now reads **blocked**, in amber, with the reason as the pill's
title so it is legible without unfolding anything.
Amber rather than red, deliberately: nothing is broken and no data was
lost — the run is waiting for the backend to return. Red reads as "this
job is failing" and invites a Cancel, which for a migration also
discards the copy already done and is the one action that cannot be
undone.
Checked BEFORE the findings branches, too. A run that never finished has
nothing meaningful to say about findings, and "0 issues" on an aborted
scan is a worse answer than "blocked".
`JobOutcome.extra` was typed `unknown`, so the panel could not read the
`retryable` flag the backend already sends. Now a narrow
`JobOutcomeExtra` exposing just the three keys that describe the RUN's
shape rather than its work — the rest stay per-job counters nothing
generic should switch on.
Same class of defect as the known "Ok despite findings" issue: an
outcome that looks like success while hiding the state an operator needs
to see.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cjw
Dionisio Pozo