75a123ae6c7cb314195f60a0ce98ae831dc63eee
436 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Edouard Vanbelle
|
75a123ae6c | feat(msg-bus): add DPoP support, fix floow from client, correct deletion | ||
|
Edouard Vanbelle
|
7918fff47b | refactor(msg-bus): prefer MessageBus as Realtime | ||
|
Edouard Vanbelle
|
1b824cb45c | feat(msg-bus): prepare engine | ||
|
Edouard Vanbelle
|
d99b718d43 |
fix(migration): counters must describe the run, not the current segment
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> |
||
|
Edouard Vanbelle
|
baee4ac9b2 |
feat(storage): a backend that never answers is now a transient failure
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>
|
||
|
Edouard Vanbelle
|
eba22f4c2c |
fix(storage): classify blob_exists too — it is the migration's first probe
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>
|
||
|
Edouard Vanbelle
|
34a2607658 |
fix(storage): a read failure is not proof the blob is gone
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>
|
||
|
Edouard Vanbelle
|
e054987c65 |
fix(migration): a transient copy failure pauses instead of skipping the blob
`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> |
||
|
Edouard Vanbelle
|
ac2cbcd963 |
fix(storage): classify backend-init failures too
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> |
||
|
Edouard Vanbelle
|
bed1d807c3 |
fix(migration): cancel releases migration_readonly, pause deliberately does not
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>
|
||
|
Edouard Vanbelle
|
303a0421c2 |
feat(jobs): a transient backend failure pauses at its cursor instead of failing
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>
|
||
|
Edouard Vanbelle
|
a7e25eea76 |
feat(jobs): PausedRetryable — an outcome the engine can act on
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>
|
||
|
Edouard Vanbelle
|
465fbe2480 |
feat(errors): classify transient failures on the type, not by string-matching
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>
|
||
|
Edouard Vanbelle
|
fc88a78055 |
fix(consistency): a mid-batch pause resumes at the last settled hash
`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> |
||
|
Edouard Vanbelle
|
9514f49eed |
feat(consistency): record what a deep run audited, and make cancel responsive
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> |
||
|
Edouard Vanbelle
|
51e3d614b2 |
fix(consistency): deep mode must verify storage, not the cache
`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> |
||
|
Edouard Vanbelle
|
a4101743e0 |
feat(jobs): jobs declare their own run parameters
`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> |
||
|
Edouard Vanbelle
|
f598404d4a |
fix(webdav): constant-time compare on lock-token equality checks
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> |
||
|
Edouard Vanbelle
|
13a2f20558 |
fix(dedup): make the chunk reap guard registry-driven too
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
|
||
|
Edouard Vanbelle
|
6dc045eaad |
fix(dedup): make the reference registry the only authority on reaping
`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`
(
|
||
|
Edouard Vanbelle
|
c9fc7dc678 |
test(dedup): pin whether ref_count alone may reap a referenced manifest
`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>
|
||
|
Dionisio Pozo
|
56da7e2365 | Merge pull request #705 from EdouardVanbelle/feat/blob_consistancy | ||
|
Dionisio Pozo
|
24a060c0cc | Merge pull request #704 from EdouardVanbelle/fix/azure-enumeration | ||
|
Edouard Vanbelle
|
abc83142b6 | feat(blob_consistancy): audit staled GC | ||
|
Edouard Vanbelle
|
49001e9beb | test(blob,manifest_consistency): sanity test on repair | ||
|
Edouard Vanbelle
|
2a629c4e8b | fix(blob_consistency): apply same repair logic as manifest_consistency | ||
|
Edouard Vanbelle
|
8a63663209 | fix(manifest_consistency): add missing derived_blob to repair | ||
|
Edouard Vanbelle
|
2b52d233f0 |
feat(azure): enumerate blobs, and fail instead of degrading when that breaks
`AzureBlobBackend` inherited the trait's `operation_not_supported`
default for `list_blob_hashes`, so every `backend_consistency` run on
Azure fell back to a per-row probe. That fallback walks `storage.blobs`
asking "are these bytes there", which structurally cannot find orphans:
bytes no row claims are invisible to anything starting from the
database, because you need a hash to ask about one and discovering
unknown hashes IS enumeration. Azure had half the coverage of local and
S3, in the direction that wastes space.
## Enumeration
The obstacle was the cursor contract. The caller advances ONE cursor
across both sides of the merge-join, feeding the same value to the
backend and to `WHERE hash > $1`, so the cursor IS a blob hash. S3
satisfies that with `StartAfter`. Azure has no equivalent on this SDK:
REST 2023-05-03 added `startFrom`, but `azure_storage_blobs` 0.21 never
sends it — `ListBlobs` exposes only prefix, delimiter, max_results and
an opaque marker that cannot be derived from a hash.
Resume rides on `prefix` instead. Names are `{hash[0..2]}/{hash}.blob`,
which partitions the container into 256 shards that are themselves in
hash order, so walking 00/…ff/ yields exactly the global order the
merge-join needs and a cursor names the shard to restart in.
Re-listing on resume is bounded by shard width rather than by the whole
container — the cost a client-side skip over a flat listing would pay on
every page. `marker` pages within one call and never escapes as the
cursor, the same treatment the S3 impl gives its continuation token.
One asymmetry against S3, deliberate: constraining to `{2-hex}/` means
foreign files outside that shape never reach `unknowns`. Safe in the
direction that matters — an orphan is a blob we wrote and stopped
referencing, so it always has the canonical name — and it buys O(N)
enumeration instead of O(N²/limit).
`hash_from_blob_name` mirrors S3's parser, shard-equals-prefix check
included: without it a mis-sharded name would round-trip to a
`blob_name` we never wrote, reporting a live blob no read path can find.
Tested for round-trip, for nine non-canonical shapes, and for the
ordering premise the merge-join rests on.
## Removing the fallback
With Azure enumerating, nothing shipped answers
`operation_not_supported`. The fallback's other stated justification —
mid-migration — never applied: it named a `MigrationBlobBackend` that
does not exist, and `SwappableBlobBackend::list_blob_hashes` forwards to
whatever is currently active, as do the Encrypted, Cached and Retry
wrappers.
What still reached it was a transient failure (auth blip, throttle,
network) relabelled as a capability limit, on a run that then read as
clean while having silently lost orphan coverage. So it was not merely
dead, it produced the wrong outcome — the only one it could. It also had
zero test coverage across 227 lines.
Now any `Err` from `list_blob_hashes` fails the run. That is louder than
an anomaly on a green run, which was the fallback's own goal. The trait
default still returns `operation_not_supported`, so a genuinely
unenumerable backend would fail every run — detectable, not silent, and
the point at which to bring the fallback back with tests.
## Also here
A doc note on `get_blob_range_stream` explaining why the Azurite
migration hang is not worked around: `azure_core` 0.21's
`Range::as_headers` attaches `x-ms-range-get-content-crc64` to any range
under 4 MiB with no opt-out, Azurite 500s on it, and `azure_core`
retries a deterministic error forever. The reachable path is
`backend_migration` → `EncryptedBlobBackend::head_check` →
`get_blob_range_stream(hash, 0, HEADER_SIZE)` — a pre-write probe on the
TARGET, so it fires on the first blob while `migration_readonly` refuses
writes app-wide. Working around it would trade production read
amplification for emulator support; the fix is the official SDK, where
`range_get_content_crc64` is an explicit field.
`RUSTSEC-2026-0275` is ignored on the same reasoning — `azure_core` 0.21
logs the `authorization` header at debug, the advisory's "upgrade to
>=0.22.0" names a version that does not exist, and the real remedy is
that same migration. Reachable only via an explicit
`RUST_LOG=…,azure_core=debug`; the entry says not to run that against a
real account.
`docs/plan/jobs-handling-recoverable-error.md` covers the other half —
a bounded retry should have turned that hang into a Paused run with a
reason, whatever the SDK does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Edouard Vanbelle
|
4baee0a1fb |
feat(transcode): key the memory cache by content, not by file
The durable tier has been content-keyed since it was introduced —
`content_derived_blobs(source_hash, kind, variant)` — but the moka cache
in front of it was still `{file_id}:{ext}`, so the layer closest to the
request used the wrong axis while the layer behind it used the right
one. That was legacy shape, and I had defended it in a comment as
"deliberate: per-request-path and short-lived", which was a
rationalisation rather than a reason. Ed asked why, and there is no why.
Transcoding is a pure function of the source bytes. Under file keying,
two files with identical content held two RAM entries for identical
bytes, and the second file was a guaranteed miss that fell through to a
DB lookup plus a blob read to fetch what was already in memory under
another key.
Now keyed by content hash when the caller has one, by file id only when
it does not — the same `content` / `external` split `ThumbnailCacheKey`
already makes, and for the same reason: hash-less callers (external
mounts) have no content identity to key on. Prefixed `c:` / `f:` so the
namespaces stay disjoint; a hash and a UUID cannot collide in practice,
but "in practice" is how a file ends up served another file's bytes.
`invalidate` now clears only the file-keyed entry. Dropping content
entries there would be wrong, not merely wasteful: one file's content
changing says nothing about the other files sharing the old bytes, and
evicting theirs would make one user's edit cost everyone else a
re-transcode. Content entries need no eviction — new content is a new
hash, so the old key is never consulted again.
transcode_cache.hurl updated to match, and its header corrected: the
second file is now a RAM hit rather than a derived-tier read, so that
scenario can no longer isolate the durable tier. It says so, and points
at satellites_consistency and a restart as what covers it instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Edouard Vanbelle
|
67032d9afa |
fix(transcode): transcode_import is on-demand, like its thumbnail twins
It was still registered on a 24h tick while the thumbnail imports moved to on-demand. The same reasoning applies and I missed it: the boot run in repair mode is the migration, nothing writes to that tree any more so the tail cannot grow afterwards, and a tick could not finish the job regardless because ticks never pass `repair`. Once drained it was a `read_dir` returning nothing, daily, forever. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edouard Vanbelle
|
10c362a94a |
fix(jobs): flush the checkpoint tail, so progress reflects reality
All three import jobs only checkpointed on a full batch, so the remainder after the last one was never counted. A run shorter than BATCH_SIZE never checkpointed at all: `scanned_count` stayed 0 against a known `total_rows`, and the admin progress bar sat at zero for the whole run and finished there. Seen on a transcode_import run over 20 entries — 13 imported, 5 negatives, 2 already present, progress 0/20 throughout. The thumbnail imports had it too, just less visibly: a 105-file run reported `scanned_count: 100`, losing the tail rather than all of it. Cursor-wise the final checkpoint is a no-op — the walk is finished, so nothing resumes from it — but the scanned delta is what the progress display reads, and it has to include the last partial batch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edouard Vanbelle
|
bf2f0dc2b2 |
fix(transcode): store the derived blob before returning, not after
Fire-and-forget raced its own purpose. A second request for the SAME content arriving before the spawned write landed found no row, re-ran the full decode + encode, and stored the identical blob again. Keying derivations by content exists so identical content is derived once — a write that has not landed yet cannot deliver that, and the window is milliseconds wide exactly when it matters most, a page loading many images at once. Caught by transcode_cache.hurl, which asserts a second distinct file with identical bytes does not re-transcode: `transcodes: 2` where 1 was expected, `disk_hits: 0` where the derived tier should have answered. It had been passing on timing luck. The cost of awaiting is bounded. This path has just spent a full decode and re-encode, so one blob write beside it is marginal, and it only runs on a genuine miss — every subsequent request for that content is served from the row. The negative verdict was already awaited, which is why only the positive half of the scenario failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edouard Vanbelle
|
71f227b737 |
feat(transcode): the local cache disables itself, and drains at boot
Completes the pattern the thumbnail migration established, for `.transcoded/`. `initialize` no longer creates the tree. Creating it at boot is exactly what kept `.thumbnails/` alive across restarts — the import removed it, the next boot put it back, and the absence the read path gates on was unreachable by construction. The write path already calls `create_dir_all` on the parent before writing, so eager creation achieved nothing except defeating the drain. It now probes instead: one `stat`, cached for the process lifetime, and the local-cache reads short-circuit on a relaxed atomic load when the tree is gone. Fails open, so a service built without `initialize` behaves as before. One difference from the thumbnail tiers, and it is not a stalled migration: callers with no content hash — external mounts — cannot use the content-keyed tier at all, so they still read and write here. On an install without such mounts the directory drains once and stays gone; on one with them it persists, correctly. `transcode_import?repair=true` joins the startup defaults on the same terms as the thumbnail imports, and with the weakest safety argument needed of the three: a transcode is a pure function of its source, so anything deleted in error is recomputed on the next request. The `default_startup_jobs` test failed on the change rather than being updated silently, which is what it is for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edouard Vanbelle
|
0e09cb81ff |
feat(transcode): transcode_import drains .transcoded/, re-keying as it goes
The twin of thumb_derived_import, with the difference that shapes the
whole job: the legacy tree is keyed by FILE (`{file_id}.webp`) while the
destination is keyed by CONTENT. Thumbnail sidecars were already named
by blob hash, so importing them was a move; every entry here has to be
resolved through storage.files first.
That re-keying is the point rather than bookkeeping. A sandbox with five
.skip markers had three of them naming the same image, so the file-keyed
tree stored one verdict three times. After the import it is one row, and
any future upload of those bytes inherits it instead of paying for the
decision again.
Both artifact kinds are claimed by one walk: `{id}.webp` becomes a
derived Blob, `{id}.webp.skip` becomes a negative row. They share a
source file and a cursor, so splitting them into two passes would be two
chances for the pair to disagree about what had been handled. `.skip` is
matched BEFORE `.webp` — the shorter suffix matches a marker too, and
getting that backwards would read a zero-byte file and store it as the
transcode of its source, then serve it to clients. There is a test.
Entries whose file is gone cannot be re-keyed at all, so they are
reported and, under repair, deleted: unimportable by definition, and a
run that keeps rediscovering them never reports zero, so the gate for
removing the directory never opens.
Deletion reuses verify_and_unlink, so a cached transcode is removed only
after its stored replacement reads back byte-identical. Directory removal
follows the same rule as .thumbnails/ — delete, and only rename aside if
a non-cache file is in the way.
The batch checkpoint is a shared helper rather than inline at both exits
of the loop body. The first draft duplicated it and dropped the future on
the skip path without awaiting: the entry counted toward the batch, the
cursor never advanced, and a resumed run would have rewalked everything
it had already handled.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Edouard Vanbelle
|
7705fca3af |
feat(transcode): count the decodes that pay nothing
Writing the hurl scenario surfaced a gap: a transcode that comes out
larger than the original runs a full decode + encode and increments no
counter at all. `transcodes` is bumped only on the success path, beside
`bytes_saved`, so the most expensive failure mode was invisible — a
multi-megapixel image decoded and re-encoded on every request, for
every file sharing that content, producing nothing.
That is precisely the cost the persisted negative verdict exists to
stop paying, and it could not be measured before or after. `not_beneficial`
counts it, kept separate from `transcodes` because conflating "work
done" with "work that paid off" would hide exactly what an operator
needs to see.
It is also what lets the hurl scenario assert the negative half: the
first fetch increments it, the second — a distinct file with identical
content — leaves it untouched, which is the negative row being read
rather than the verdict recomputed.
Assertions are exact equality against captured values throughout, no
`>` or `<`. A "greater than" would pass if a counter moved for the
wrong reason; equality against the prior reading catches any transcode
from any source, including one this scenario did not intend to cause.
Also fixes two URLs the first runs caught: file download is
`GET /api/files/{id}`, not `/content`, and the trash listing is
`/api/trash/resources`. And the duplicate uploads go to a second
folder — re-uploading the same filename into the same folder returns
the EXISTING file id, which would have made both halves of every
"two files, one content" pair the same row and left the scenario
asserting nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Edouard Vanbelle
|
8fabbfad9e |
test(transcode): a fixture the WebP encoder cannot shrink
The transcode negative path — "the result came out larger, serve the original and remember that" — had no test because no synthetic image reaches it. Measured against the real encoder: flat colour goes 4780 → 186 bytes, a diagonal gradient 24852 → 102, and uniform RGBA noise still loses by ~242 bytes at every size, a margin constant in absolute terms and so one that never flips. Grayscale does not help either; WebP's subtract-green transform handles R=G=B. Two things have to be true at once and only real content does both. The encoder is the `image` crate's own minimal VP8L writer, not libwebp, so it wins only where redundancy is extreme enough for any encoder to find it. And the original has to be near PNG-optimal, which a screenshot from a real capture tool is: a 2x Retina UI is long identical runs, flat panels and sharp edges — precisely what PNG's scanline filters plus zlib were built for. So the fixture is a real OxiCloud screenshot (emails masked by overtyping rather than block-filling, which would have added back the flat redundancy the property depends on; re-verified negative after masking, 556180 -> 511124 bytes). `fixture_premise` pins both halves of what tests/api/transcode_cache.hurl will assume — this one negative, red-image.png positive. Without the guard a future encoder bump would silently turn the negative half of that scenario into a second positive test: still passing, no longer checking what it was written to check. Worth recording for whenever libwebp replaces this encoder: most of these screenshots would likely flip to positive, which leaves every stored negative row a stale verdict. An encoder change has to purge them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edouard Vanbelle
|
9c63f9969a |
feat(transcode): write transcodes to the derived tier, with negative rows
Step 7 of docs/plan/derived-blobs.md, write path first — the plan is explicit that fixing it before the import means transcode_import only has to handle history, not a moving target. ImageTranscodeService now reads and writes storage.content_derived_blobs under kind='transcode', keyed by the BLAKE3 of the SOURCE content. The hash is threaded in from file_retrieval_service, which already holds it as dto.content_hash; hashing here would be a BLAKE3 over the whole file on every request. Callers without one (external mounts) keep the local cache untouched, which is what the service did before this tier existed. Negative verdicts become rows rather than zero-byte .skip files. A transcode that came out larger is deterministic in the content, so it is worth remembering; the row survives moka eviction, a restart, and the deletion of .transcoded/, none of which the marker does. Only that verdict is persisted — a timeout or a read error returns Err and is recorded nowhere, because a momentary failure written here would mark a perfectly transcodable image hopeless with nothing to retry it. Representation is a NULL blob_hash, per the plan: a sentinel hash would stop blob_hash naming a real Blob and every consumer would need to learn the exception. A CHECK keeps blob_hash and content_type NULL together — a type without bytes describes nothing, bytes without a type cannot be served. Two consumers had to be corrected for NULLs first, both of which would have broken on the first negative row ever written: * satellites_consistency reported them as derived_dangling_blob at data_loss severity. SQL comparison against NULL is NULL, so EXISTS was false and a row correctly pointing at nothing read as an artifact that had gone missing. * blob_reference_sources::list_referenced_blobs decodes blob_hash into String, so the first NULL would have failed the decode and taken the whole enumeration down. It would also have been wrong if it decoded — a negative row holds no reference, which is why the counting forms (WHERE blob_hash = <hash>) already exclude it for free. lookup_derived returns a three-way answer because Option collapses the two cases a caller deciding whether to spend a decode most needs apart: never attempted, versus attempted and known not worth it. DedupService is attached after construction via a OnceLock. DI builds the transcode service ~240 lines before DedupService exists, and the retrieval path that needs it is wired earlier still, so a constructor argument would mean reordering more than this is worth. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edouard Vanbelle
|
fb0925d10a |
fix(thumbnails): a drained tier is not a teardown failure
Every boot after the migration completes logged `WARN legacy sidecar directory could not be removed / No such file or directory`. The directory being absent IS the end state — it is what success looks like from the second boot onward — so this warned about the migration having worked, forever, on every restart. Returns early when the root is gone, which also skips walking three directories that no longer exist. The remaining `Err` arms keep their warning for the cases that are genuinely failures: a directory that exists and cannot be removed or moved aside. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edouard Vanbelle
|
ce4354f497 |
fix(thumbnails): neither import job may tear down the shared directory
Found on a sandbox restore. `thumb_derived_import` ran first, imported and deleted its own hash-named sidecars, then found `remove_dir` refused because the `ext-*.jpg` previews were still there — those belong to `thumb_attached_import`. The rename fallback fired, moving the tree to `.thumbnails.migrated`; the attached job then looked in `.thumbnails/`, found nothing, and reported zeros. That stranded the user-uploaded previews, which are the one class of file here with no render path to rebuild them. The rename exists for files NEITHER job claims — a `.DS_Store` blocking removal forever — and it fired for the sibling's work in progress instead. Inverting the job order does not help: once the tree is renamed, both jobs look at `.thumbnails/` and find nothing, whatever order they run in. Teardown is now shared and refuses to act while anything remains that either job would claim. Both jobs call it, so whichever finishes last removes the tree in the same boot rather than leaving an empty directory until the next one. The rename survives for its original purpose, and now only fires when the remaining files are genuinely nobody's. Also drops the daily tick on both imports — they are on-demand now. The boot run in repair mode IS the migration: nothing has written a sidecar since step 10d2, so the tail cannot grow afterwards, and a tick could not finish the job anyway because ticks never pass `repair`. Once drained it was a `read_dir` returning nothing, every day, forever. UX: the "at boot" badge moves from beside the job name into the cadence column. It answers WHEN a job runs, which is what that column is for — next to the name it read as a property of the job, and the row could show "on-demand" beside a badge saying otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edouard Vanbelle
|
577ecb7cef |
feat(jobs): run the thumbnail migration at startup, by default
A migration nobody triggers never finishes. Scheduled ticks deliberately never pass `repair`, so a deployment whose operator never opens the admin panel re-imported the same sidecars forever and never drained the directory — and relying on operators to edit `.env` has the same failure mode one level up. `OXICLOUD_STARTUP_JOBS` dispatches named jobs once, in the background, after the scheduler is ready. Entries use the syntax operators already type at the trigger URL (`name?repair=true`), so the value is literally the request they would otherwise make by hand. It defaults to both migration jobs in repair mode, so an untouched deployment migrates and drains itself. That is a destructive default and a real exception to no-silent-auto-repair, so the guard it rests on had to get stronger: `verify_and_unlink` now compares CONTENT, not length. A blob of the right size and the wrong bytes used to pass — a key-mapping bug handing back another file's preview at the same length would have deleted the original and kept the impostor, and thumbnails cluster tightly enough in size for that to be a real coincidence. The readback streams from the backend with no cache in front, so it proves durability rather than that a write was acknowledged. Deletion of `.thumbnails/` is attempted first and only falls back to renaming it `.thumbnails.migrated` when `remove_dir` refuses because a non-sidecar file is inside (Finder's `.DS_Store`). Either way the directory stops existing, which lets the read-path probe go back to a single `stat` on the root instead of walking the size directories. Validation is fail-fast: an unknown job name or flag panics at boot. A silently dropped `?repare=true` would leave the job in discovery-only mode while the operator believed the tier was draining, surfacing months later as "the migration never finished" with nothing pointing at the config line. Interrupted runs resume. Boot recovery flips abandoned rows to Paused with their cursor, so `run_or_resume` continues rather than rescanning — a long migration completes across however many restarts it takes. That is a scoped exception to "we do not auto-resume": here somebody did ask, in configuration, and not having to ask again is the point. `StartupJob` holds a `JobRunArgs` rather than re-listing its four fields, so a fifth flag cannot be added to the scheduler and silently ignored in configuration. Jobs named here are ordinary registered jobs — visible in the panel, triggerable by hand, same runs and findings. Their rows now carry a `startup` object so an operator can see that a job deletes on every boot rather than only when someone clicks Run. Adds docs/config/thumbnail-migration.md: what runs on first boot, how to snapshot database and storage together beforehand, and how to verify afterwards with satellites_consistency plus backend_consistency ?deep=true. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edouard Vanbelle
|
03246305f6 |
feat(thumbnails): the sidecar fallback disables itself
Step 10e was written as a removal release: delete the fallback read path once the directories are empty. That has the same flaw as gating deletion on an empty tail, one level up — sidecars are local disk, so no release can know that every instance has drained. The only removal that can actually be written is "if the tier is gone, return". `initialize` now probes the size directories once at boot; when absent, every fallback read short-circuits on a relaxed atomic load and touches no filesystem. The code stays, costs nothing, and can be deleted whenever — or never. Two things had to change for absence to be reachable at all: * `initialize` no longer creates the directories. It create_dir_all-ed all three at every boot, so the import job removed them and the next restart put them back — the absence this gates on was unreachable by construction. Found on a sandbox where the job had drained the tier and a restart left three empty directories behind. Nothing has written a sidecar since step 10d2, so there was nothing to create them for. * The probe tests the size directories, not the root. On macOS Finder leaves a .DS_Store in the root, which blocks remove_dir there permanently; gating on the root would keep the fallback alive on every developer machine for a reason unrelated to thumbnails. No size directory means no sidecar. Every sidecar read and existence check now goes through `read_sidecar` / `sidecar_exists`, so the guard exists once rather than at each of the twelve sites that built a path and read it — the build-then-read pair was duplicated six times over. The import job's root removal reports its outcome instead of discarding it. It is the one result an operator is waiting for, and "directory not empty" with no sidecars left is a failure worth naming. Falls open: the flag starts true, so a service constructed without `initialize` behaves as before. A drain completing mid-process leaves it stale-true until restart, which costs the same failed opens as today; it never goes false while sidecars remain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edouard Vanbelle
|
f1f327a6c4 |
refactor(consistency): blobs_consistency reads only the database
`blobs_consistency` probed `blob_exists` once per row and, under `?deep=true`, read and re-hashed every blob. `backend_consistency` already reports the same `blob_missing_from_backend` from its merge-join — so the probe was duplicated work that found strictly less (a DB walk cannot see backend-only orphans by construction) at N round -trips instead of one enumeration. Every scheduled sweep paid for it. All three physical checks move to `backend_consistency`: * `blob_missing_from_backend` was already there; the duplicate is gone. * `blob_corrupted` / `blob_unreadable` hook the matched arm of the merge-join, which holds exactly the key pairs worth reading. Guarded by `in_range` so a pair past the horizon is not read twice, and `params.deep` is persisted on a fresh run and read back on resume so a paused deep scan does not silently continue shallow. Deep mode belongs there because it is backend work end to end: the only DB input is the hash. Keeping it in `blobs_consistency` forced that tenant to carry a backend for one flag. What remains is the half that needs no backend: `refcount_mismatch` and its repair. The constructor drops from five parameters to two — no backend, no storage_entries, no storage_path_fallback — and `?storage=<name>` / `?deep=true` are now inert there, which the job description says outright. `affected_files` is needed by both tenants, so it moves to a shared `blob_diagnostics` module rather than being copied. `PROBED_STORAGE_PARAM` moves to `backend_consistency`: it was defined in `blobs_consistency` and re-exported, which is backwards once the DB-only tenant has no entry to scope. The create-grace window goes with the probe — it existed to avoid flagging a blob whose bytes had landed before its row, and the refcount comparison reads one consistent snapshot. Known cost: `backend_consistency` returns `backend_unenumerable` on Azure and mid-migration, so on those configs missing bytes now go unreported where the per-row probe caught them. That argues for the Azure enumeration impl, not for keeping the probe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Edouard Vanbelle
|
1ea3826660 |
feat(jobs): jobs describe themselves — description, mutates, repair_description
The admin panel had no repair toggle wired to anything but a hardcoded
name list naming the two refcount tenants, so `thumb_derived_import` and
`thumb_attached_import` could not be run in repair mode from the UI at
all despite supporting it. And nothing in the job list said what any
given job does or whether clicking Run on production writes anything.
Three defaulted methods on `JobHandler` and `RecoverableJobHandler`:
fn description(&self) -> &'static str
fn mutates(&self) -> Mutates // Never | Always | OnRepairOnly
fn repair_description(&self) -> Option<&'static str>
`RecoverableAdapter` forwards them — the registry only holds
`dyn JobHandler`, so a tenant's metadata is invisible otherwise, and
falling back to the defaults would report every recoverable job as
read-only, including the ones that delete files.
Three values rather than a boolean because a job can be read-only by
default and destructive under `?repair=true`; a boolean answers wrongly
for one of its two modes, and `false` on something that unlinks files is
the dangerous direction to be wrong in. `repair_description` returning
`Option` collapses "does it repair" and "what does repair do" into one
method: presence gates the toggle, content is the confirmation text —
which the frontend cannot invent, since correcting a counter and
deleting sidecars are not the same warning.
`OnRepairOnly` with no `repair_description` is rejected at registration:
it claims to mutate only under a flag it does not support.
All 17 registered jobs declare all three. The panel now renders the
description under each name, badges read-only jobs, confirms before a
plain run of a mutating one, and offers the repair variant off the
backend flag instead of the name list.
Descriptions are English in the trait, next to the behaviour: one in
`locales/*.json` rots invisibly the moment a job changes, and a
translator cannot know what `manifests_consistency` reconciles. i18n can
layer on later keyed by job name with these as the fallback.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Edouard Vanbelle
|
b485db46fa |
feat(storage): audit every sidecar deletion, and reclaim orphaned uploads
Two changes to the import jobs' destructive path. thumb_attached_import now deletes orphaned sidecars under `repair`, matching the dead-source case on the derived side. An `ext-` file whose owner is gone is unimportable — the FK on file_id would reject the row — so leaving it means it is rediscovered every run, the tail never empties and step 10e's gate never opens. Safe despite these being the non-regenerable bytes: the preview is keyed to a file_id that no longer exists, so nothing can reference it again. Unrecoverable and unreachable are different things, and this is both. And every deletion is now audited. A one-way migration removing user-visible files should leave a trail that outlives the run history: findings are per-run and get purged, whereas target: "audit" is separable and retained. If a preview later turns out to be missing, this is the only record saying the migration removed it and when. `owner` carries the id the file belonged to — source_hash for content-keyed, file_id for uploaded — because that is where an investigation starts, and the raw logs cannot supply it: NEW BLOB names the hash of the STORED BYTES, a different value from the sidecar's own name, which is why grepping one against the other finds nothing. reason is a stable key: `imported` (replaced by a verified blob), `source_gone`, `orphaned`. The first lives inside verify_and_unlink so a verified deletion cannot be logged inconsistently; the other two are explicit, since those paths have nothing to verify against. |
||
|
Edouard Vanbelle
|
1a3d7d201a |
fix(storage): skip sidecars whose source is gone, before writing anything
Running the import on a real install produced a store-then-discard loop: NEW BLOB (CDC) immediately followed by MANIFEST DELETED, once per sidecar. store_derived_blob wrote the bytes, the source-exists guard refused the row, and `inserted == 0` released the reference again. The refusal is right — `.thumbnails/` outlives years of deleted files, and importing those would recreate exactly the orphan rows e4c78ae0 eliminated. The mistake was deciding it AFTER the write. Now checked before the read and the store, via blob_exists (manifest first, blob as fallback). Two costs it removes: a blob write plus a manifest delete per dead sidecar on EVERY run, and a tail that never empties — unimportable files are rediscovered forever, so the job never reports zero and step 10e's gate never opens. Reported as `sidecar_source_gone` so the scale is visible before anything is removed, and deleted under `repair`. That is the one unlink in this job needing no readback: there is nothing to read back and nothing to regenerate from. Counted separately in the completion log, because "skipped, source gone" and "already present" mean different things to an operator deciding whether the migration has converged. Worth noting for anyone reading the raw logs: NEW BLOB names the hash of the STORED BYTES, while the sidecar filename is the SOURCE hash. They are different values, so grepping the log hash against .thumbnails finds nothing. The new finding carries both. |
||
|
Edouard Vanbelle
|
b3221e265d |
feat(consistency): satellites_consistency covers both tables, and the sweep covers every job
Extends the derived check to `file_attached_blobs` and renames it, since the two tables are one concept — the content-keyed and file-keyed halves of "things attached to a Blob" — and `storage.copy_file_satellites` already established the vocabulary. The attached half is the one that cannot be recovered. `attached_dangling_blob` is data_loss with `recoverable: false`: those bytes were user-supplied and have no server-side render path, so nothing can regenerate them. Its derived twin carries `recoverable: true`, because a derived artifact is a pure function of its source and re-rendering restores it. Same finding shape, materially different stakes, and the detail says which. No orphan-mapping check on the attached side, deliberately: `file_id` is ON DELETE CASCADE, so a row cannot outlive its file. The database enforces what the derived table cannot, since a content hash has no row to point a foreign key at — which is exactly why only that half could rot. One job walking two tables needs a phase in the cursor, or an attached checkpoint would be replayed against the derived table and silently re-scan or skip. Two things the sweep was missing, found while checking whether every consistency job is actually exercised: drives_consistency and folders_consistency were registered but never run by any test. Now included; the list is exhaustive by intent. An unknown job was a warning-and-skip. That protected feature-gated builds at the cost of something worse: this list said `derived_consistency` for one commit after the rename and would have dropped that coverage without a word, leaving the suite green over a check that no longer ran. It fails now. |
||
|
Edouard Vanbelle
|
4fef34b230 |
feat(consistency): derived_consistency — the last coverage-matrix gap
Finds derived mappings whose Blob is gone on either side. Nothing else can, and that is the point rather than an oversight: every other job reasons from a Blob outwards, so a row whose SOURCE was reaped breaks none of their invariants — valid reference, exactly correct refcount, bytes present on the backend. Every check agrees the system is healthy while the artifact is pinned forever. A leak that looks like correctness, which is why it took four suite runs to name. Two findings: derived_orphan_mapping (inconsistent) — source_hash has neither a manifest nor a blob row, so purge_derived_blobs can never fire for it. Storage that grows and never reclaims. derived_dangling_blob (data_loss) — blob_hash has no Blob behind it. The mapping promises an artifact that is gone, so a read finds the row and then fails. Existence means EITHER table on both sides, since source_hash and blob_hash each name a Blob: a manifest for CDC content, a bare blob row for legacy whole-file content. Checking one would report every legacy blob as missing. Paged on the full primary key with a row-value comparison rather than source_hash alone — a source has several variants, so a page boundary can fall inside one and advancing by source would skip the rest. Both existence probes fold into the page query, so a page is one round-trip rather than 2xN. Cursor round-trip is tested, including that a malformed one fails loudly: silently restarting would make a paged audit under-report, which is the worst failure available to a job whose purpose is finding what is missing. e4c78ae0 stops new orphans at the write side; this finds the ones already on disk, which that fix cannot reach. Added to the end-of-suite sweep so it runs against real state every time. |
||
|
Edouard Vanbelle
|
de0f625d4c |
fix(dedup): refuse a derived mapping whose source is already gone
Permanent blob leak, three rows per image. Confirmed green after this. The leftovers named their source, and it had no manifest, no blob row and no files. Nothing will ever reap that hash again, so purge_derived_blobs can never fire for it — meaning the rows were written AFTER the source died, not left behind by a reap that skipped them. Two earlier attempts assumed the latter and fixed the wrong thing. Background thumbnail generation is spawned and unawaited, so an upload deleted promptly — constant in a test suite, occasional for real users — has its render finish after GC reaped the blob and then record three mappings to a corpse. Each pins its own thumbnail blob at ref_count 1, which GC is thereafter CORRECT to refuse: that is why three passes with force=true reclaimed nothing and why the leak was invisible, a healthy system declining to delete referenced data. store_derived_blob now inserts only WHERE the source still exists, checking both tables since source_hash names a Blob — a manifest for CDC content, a bare blob row for legacy whole-file content. A refused insert falls into the existing `inserted == 0` branch and releases the reference, so the thumbnail blob becomes collectible rather than stranded. Closed in both directions: if the source dies before the statement's snapshot the row is refused; if after, that reap's purge finds the row. 034f1050 stays — the bulk manifest reap genuinely lacked the purge that reap_blob had, and two manifest reap paths with only one purging is its own defect. It just was not this one. Still missing, and now clearly worth building: the orphan-mapping check the plan's coverage matrix already lists (content_derived_blobs. source_hash with no Blob behind it). This stops new ones; nothing yet finds the ones already on disk. |
||
|
Edouard Vanbelle
|
ea5d3003e0 |
fix(dedup): bulk manifest reap orphaned every derived row
Real leak, found by storage_cleanup_check.sh: three blobs surviving a full teardown, all `derived=1`, all naming one `src` whose manifest, blob row and files were already gone. The source had been reaped without its derived rows being purged. `reap_blob` purges correctly for the single-blob path. The BULK manifest reap did not — it iterated the deleted batch only to invalidate the manifest cache, so every manifest reaped that way left its content_derived_blobs rows behind. The predicate is not at fault. It protects a manifest that IS a derived artifact (content_derived_blobs.blob_hash) and deliberately not one that is the SOURCE of them, because counting source_hash as a reference would pin every original for as long as a thumbnail existed. The source is therefore reaped correctly and the purge simply has to follow it. The consequence is permanent, not cosmetic: the orphaned row holds chunk_manifests.ref_count at 1 on the thumbnail's own blob, so GC is thereafter CORRECT to refuse it — which is why three passes with force=true reclaimed nothing. Every deleted image left three behind, one per size, growing forever. Fixed at the reap rather than in any deletion path, which is where all of them converge: folder cascade, drive deletion, user deletion and single-file delete all reach it through the decrement trigger, so one call covers every route. |
||
|
Edouard Vanbelle
|
1b68ee093e |
feat(storage): both import jobs tick daily instead of manual-only
Registered with interval None, so they ran only when someone remembered to trigger them — which was your objection to gating anything on operator timing. Now daily. Not boot-time: that would delay readiness for a filesystem walk, and both jobs are idempotent and resumable, so periodic is strictly better. The tick deliberately does NOT delete. `repair` defaults false, so scheduled runs import and stop; unlinking stays a deliberate operator action, per no-silent-auto-repair. That splits the two halves the way their risk differs — the backfill is safe to automate, removing files is not. Cost once drained is a read_dir over three directories returning nothing, and after the directory itself is removed, not even that. |