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