diff --git a/.cargo/audit.toml b/.cargo/audit.toml index 8b2a030a..967d4db7 100644 --- a/.cargo/audit.toml +++ b/.cargo/audit.toml @@ -51,6 +51,33 @@ ignore = [ # No direct security impact; no upgrade path exists. "RUSTSEC-2024-0384", + # azure_core 0.21.0 writes the `authorization` header value to logs — + # `policies/transport.rs` does `debug!("…{request:#?}")` over the whole + # request. For a SharedKey entry that value is the per-request HMAC + # signature; for a SAS entry it is the token. Severity 6.5 (medium). + # + # The advisory says "upgrade to >=0.22.0". That version does not exist: + # `azure_core` jumped 0.21 → 1.x, and `azure_storage_blobs` never left + # 0.21.0 before being archived. So the stated remedy IS the official-SDK + # migration, tracked separately alongside the quick-xml pair above. + # + # Not reachable at our log levels: the line is `debug!` on the + # `azure_core::policies::transport` target, and the default filter is + # `info`. It fires only if an operator explicitly asks for + # `RUST_LOG=…,azure_core=debug`, which is not hypothetical — that is the + # invocation used to diagnose the Azurite migration hang. **Do not run + # `azure_core=debug` against a real Azure account**; it prints request + # signatures to the terminal. Against Azurite it only exposes the + # published dev key's signatures. + # + # A subscriber-level directive pinning that target off was prototyped + # and rejected 2026-09-02 — not worth carrying a filter hack for a + # dependency being replaced. + # + # Un-ignore trigger: the azure_storage_blob 1.x migration lands + # (`cargo tree -i azure_core@0.21` returns no rows). + "RUSTSEC-2026-0275", + # quick-xml 0.31.0 — transitive via azure_core 0.21.0 (unofficial SDK, # now archived). Our direct dep is already on 0.41.0; the 0.31 copy is # only reachable through the azure_storage_blobs chain, which parses diff --git a/docs/plan/jobs-handling-recoverable-error.md b/docs/plan/jobs-handling-recoverable-error.md new file mode 100644 index 00000000..3337c0d4 --- /dev/null +++ b/docs/plan/jobs-handling-recoverable-error.md @@ -0,0 +1,265 @@ +# Recoverable errors in jobs — retry, then pause + +**Status: not started.** Design settled 2026-08-31, from a live +diagnosis (see [Motivating incident](#motivating-incident)). + +A job that hits a failing backend today has two possible endings, and +neither is right for an outage: it fails the run (throwing away a +partially-complete migration, since `Failed` is terminal and only +`Paused` resumes), or it hangs forever inside an SDK retry loop with no +log line and no way to act on it. + +This plan adds the third: **retry a bounded number of times, then pause +with the reason recorded**, so an operator resumes when the provider +recovers and the job continues from its cursor. + +--- + +## Motivating incident + +`backend_migration ?storage=azurite` hung indefinitely. Diagnosis, after +several wrong theories: + +- The job issued a ranged GET carrying `x-ms-range-get-content-crc64`. +- Azurite answered **500** (real Azure supports CRC64 range validation; + the emulator does not). +- `azure_core`'s retry policy classifies 500 as retryable and loops. +- The response was deterministic, so every retry failed identically. +- The job never advanced, never failed, and emitted no per-blob line — + while holding `migration_readonly`, refusing writes **across the whole + application**. + +The exact chain was pinned down later (2026-09-02) and is worth having, +because it is not where you would look — the copy itself is innocent: + +``` +backend_migration_service.rs target.head_check(hash) ← pre-write probe + → EncryptedBlobBackend::head_check + → get_blob_range_stream(hash, 0, HEADER_SIZE) ← ~40 bytes + → azure_core Range::as_headers ← adds the CRC64 + (src/request_options/range.rs: any range < 4 MiB) header, no opt-out +``` + +`copy_blob` reads from the SOURCE, which is local in a local→Azure +migration, so it never touches an Azure range. What hangs is the format +probe against the TARGET, on the first blob, before a byte is copied. + +A workaround exists — unranged `get()` for small requests, truncate +client-side — and was **rejected 2026-09-02**: it pays for an emulator +with production read amplification and puts new offset arithmetic on the +read path. See the note on `AzureBlobBackend::get_blob_range_stream`. +The fix is the official SDK, where `range_get_content_crc64` is an +explicit field. **This plan is unaffected either way** — a bounded retry +would have turned the hang into a Paused run with a reason, which is the +point. + +Two properties made it invisible: the 500 was only visible at +`azure_core=debug`, and nothing bounded the retry. The same shape would +occur against real Azure or S3 on any persistent 5xx; it is not an +emulator quirk. `AzureBlobBackend` configures no retry policy and no +timeout at all — `grep "retry\|timeout\|ClientOptions"` on +`azure_blob_backend.rs` returns nothing. + +Full evidence chain, including the theories ruled out and what each +cost, is in the memory note `bug-azure-put-blob-hangs-no-timeout`. + +--- + +## Why this belongs at the top level + +Every recoverable job goes through `run_or_resume`, which already owns +the run lifecycle: it opens the row, persists and restores +[`JobRunArgs`](./job-registry.md) (`63820c3c`), dispatches the handler, +and writes the terminal state. Retry-and-pause is the same kind of +concern — policy about *how a run behaves*, not about what any one job +does. + +Implemented there, `backend_rotate`, `transcode_import`, the thumbnail +imports and anything added later inherit it. Implemented per-job, it +gets written once per job and drifts. + +It also survives the pending official-Azure-SDK migration untouched, +where per-SDK retry tuning would have to be redone. + +**The blocker is that the engine cannot currently act on what it is +told.** A handler returns `RunOutcome::{Completed, Paused, Failed}`, so +a transient backend error is already flattened into `Failed` before the +engine sees it — "the provider is down" and "this data is wrong" are +indistinguishable. Closing that is what makes a top-level +implementation possible, and it is step 1. + +--- + +## Step 1 — errors say whether they are retryable + +Today both backends wrap SDK errors into +`DomainError::internal_error("Azure", format!("…{e}"))`, so the status +code survives only inside a formatted string. Recovering it means +string-matching, which is exactly the kind of fragility that turns into +a silent behaviour change when an SDK reformats its `Display`. + +Carry the distinction on the type instead — a `retryable` flag, or a +kind the storage ports set deliberately. + +**Retryable** (environment, may clear on its own): +- HTTP 5xx, 429 / `SlowDown` / throttling +- connect timeouts, connection resets, DNS failure + +**Permanent** (will fail identically forever): +- 4xx other than 429 — 401/403 (credentials), 404 (missing container) +- decode failures, checksum mismatch +- `operation_not_supported` + +> **The Azurite 500 is a permanent error wearing a retryable status +> code.** No classification by status alone gets this right, which is +> the case for a bounded cap rather than "retry until it works". The cap +> is the safety net for exactly the errors the taxonomy misjudges. + +**Do not double-retry.** The AWS SDK already retries internally with its +own backoff, so a second layer above it multiplies. Check what the S3 +backend inherits before adding anything, and consider making the +engine's cap the *outer* bound with SDK retries reduced or disabled. + +## Step 2 — an outcome the engine can act on + +`RunOutcome` grows a variant meaning "the environment failed, this is +worth trying again later": + +```rust +RunOutcome::PausedRetryable { cursor: Vec, reason: String } +``` + +Distinct from all three existing outcomes, and the distinction is the +point: + +| outcome | meaning | resumes? | +|---|---|---| +| `Failed` | the data or the request is wrong | no — terminal | +| `Paused` | an operator asked it to stop | yes | +| `PausedRetryable` | the environment failed | yes, and says why | + +The row lands as `Paused` either way, so resume works unchanged. What +differs is `error_message`, which must let the panel — and an operator — +tell "I paused this" from "the provider went down". Without that +distinction a paused run is an unexplained one. + +## Step 3 — the engine implements the policy + +In `run_or_resume`: + +- bounded exponential backoff, ~5 attempts +- **log each failed attempt at `warn` on our side.** The incident took + several runs to diagnose because the 500 was visible only at + `azure_core=debug`. One line per exhausted operation, naming status, + target and attempt count. +- on exhaustion, write the row as `Paused` with the reason in + `error_message` + +Handlers then return the retryable outcome and get the policy for free. + +--- + +## Step 4 — `migration_readonly`, the sharp edge + +`backend_migration` holds a gate that refuses writes **application-wide** +until cutover. What happens to it on pause is a correctness question, +not a cosmetic one. + +### Start conservative: keep the gate held while paused + +Correct, because no writes during the pause means the cursor stays valid +and resume-from-cursor is sound. + +It is also **strictly better than today**, which is the thing to +remember when the read-only window looks unattractive: the application +is *already* read-only while the job hangs — there is simply no way to +see why or act. Same lock, now with a reason and two operator actions. + +**Cancel must clear the gate.** It ends the run with no swap, so the +source stays active and writes must return. The `failed > 0` path +already clears readonly on the reasoning that "users shouldn't be locked +out because of a partial run" — cancel is the escape hatch operators +will reach for during an outage, and it must work. + +**Make the state loud.** Writes refused app-wide should be obvious in +the admin panel, not discovered by reading a run row. The progress +snapshot already feeds the header middleware; a paused migration wants +the same visibility, saying why and offering resume/cancel. + +### Why NOT to release the gate on pause (yet) + +Tempting — ops should not be locked out during a provider outage — and +unsafe as the job stands: + +1. Release readonly; users write again. New blobs land on the source, + which is still active, and are absent from the target. +2. Resume continues **from the cursor**, a position in a hash-ordered + walk. +3. A blob written during the pause whose hash sorts *before* that cursor + is never visited. +4. The run completes, flips the pointer, and reads for that hash 404 + against a target that never received it. + +This is why the existing `failed > 0` path can safely clear readonly: it +**ends** the run, and an operator retrying starts a new run with a fresh +cursor, so everything is rescanned. Pause-and-resume is what +reintroduces the gap. + +Releasing on pause becomes safe only alongside one of: + +- **resume rescans from the beginning** rather than trusting the cursor + — cheap, because the walk short-circuits on blobs already present in + the target, so a second pass costs a lookup per blob, not a copy; or +- **a final catch-up pass under readonly before the swap**, with the + pointer flipping only when a complete pass finds nothing new. + +### Where this should end up + +The second option is the standard online-migration shape, and it is +where this job wants to go regardless: copy the bulk **without** holding +the gate, engage it only for a short final catch-up plus the swap. + +That removes what made the incident damaging — writes refused app-wide +for the entire duration of a long copy — and turns releasing-on-pause +into a free consequence rather than a correctness fix. Worth doing as a +follow-up, from the safer base this plan establishes. + +--- + +## Scope: Azure and S3 both + +The only reason S3 looks healthy is that the endpoint behaves. A +persistent 5xx from S3 hangs identically — the gap is the absence of a +bound, not anything Azure-specific. + +Putting the policy above the `BlobStorageBackend` trait covers Azure, +S3, local and anything added later with one implementation. Per-backend +SDK retry tuning stays a separate, optional refinement — and for Azure +specifically it should wait for the official SDK, since `azure_core` +0.21 is archived and queued for replacement. + +## Testing + +The `backend_consistency_azure.hurl` scenario and its Azurite service +are already wired (`tests/common/docker-compose.test.yml`, +`spawn-db.sh`), which gives a backend that reliably produces the +failure: Azurite 500s on the CRC64 ranged GET every time. That makes it +a genuine fixture for this work rather than a flake — +**deterministically unretryable-but-retryable-looking**, which is the +hard case. + +Note the scenario no longer triggers a migration — it audits Azurite +without cutting over, for the reason in its header. Reaching the fixture +means triggering `backend_migration ?storage=azurite` explicitly, which +is exactly the hang this plan is meant to convert into a Paused run. +Doing that inside the shared suite is what ordering it last was for; it +can go back once the outcome is bounded. + +Assert the run reaches `Paused`, that `error_message` names the cause, +and that it does so in bounded time rather than hanging. + +`POST /api/admin/settings/storage/test` with `entry_name` is the +pre-flight worth keeping in mind: synchronous, does a real write/read +round-trip, and isolates "the backend is misconfigured" from "the job +is broken". It passing while the migration hung is what ruled out +credentials, container and the write path during the incident. diff --git a/src/infrastructure/services/azure_blob_backend.rs b/src/infrastructure/services/azure_blob_backend.rs index 622af000..e8a18b43 100644 --- a/src/infrastructure/services/azure_blob_backend.rs +++ b/src/infrastructure/services/azure_blob_backend.rs @@ -13,7 +13,8 @@ use futures::{StreamExt, TryStreamExt}; use tokio::fs; use crate::application::ports::blob_storage_ports::{ - BlobStorageBackend, BlobStream, StorageHealthStatus, + BackendBlobEntry, BackendUnknownEntry, BlobListPage, BlobStorageBackend, BlobStream, + StorageHealthStatus, }; use crate::common::config::AzureStorageConfig; use crate::domain::errors::{DomainError, ErrorKind}; @@ -55,6 +56,29 @@ impl AzureBlobBackend { } } + /// Inverse of [`Self::blob_name`] — the hash a listing entry names, + /// or `None` when the entry is not one of ours. + /// + /// Mirrors `S3BlobBackend::hash_from_object_key`, including the check + /// that the shard equals the hash's own first two characters: without + /// it, `blob_name(hash)` would not reproduce the name we just parsed, + /// and a mis-sharded object would be reported as a live blob that no + /// read path can find. + fn hash_from_blob_name(name: &str) -> Option { + let (prefix, rest) = name.split_once('/')?; + if prefix.len() != 2 || !prefix.chars().all(|c| c.is_ascii_hexdigit()) { + return None; + } + let stem = rest.strip_suffix(".blob")?; + if stem.len() != 64 || !stem.chars().all(|c| c.is_ascii_hexdigit()) { + return None; + } + if !stem.starts_with(prefix) { + return None; + } + Some(stem.to_string()) + } + /// Compute the blob name for a given hash. fn blob_name(hash: &str) -> String { let prefix = &hash[0..2]; @@ -243,6 +267,37 @@ impl BlobStorageBackend for AzureBlobBackend { }) } + /// # Known incompatibility: sub-4 MiB ranges break on Azurite + /// + /// `azure_core` 0.21's `Range::as_headers` + /// (`src/request_options/range.rs`) attaches + /// `x-ms-range-get-content-crc64: true` to **any range shorter than + /// 4 MiB**, unconditionally and with no opt-out. Real Azure honours + /// it; Azurite answers 500. `azure_core` then classifies 500 as + /// retryable and loops on a deterministic error, forever. + /// + /// The reachable path is `backend_migration` → + /// `EncryptedBlobBackend::head_check` → + /// `get_blob_range_stream(hash, 0, HEADER_SIZE)`. `HEADER_SIZE` is a + /// few dozen bytes, and it runs against the TARGET before each write, + /// so a local→Azurite migration hangs on its first blob while holding + /// `migration_readonly` — writes refused application-wide. + /// + /// **Deliberately not worked around here.** The available workaround + /// is to issue an unranged `get()` for small requests (its 16 MiB + /// `initial_range` clears the threshold, so the header is never sent) + /// and truncate client-side. That is correct against real Azure but + /// pays for an emulator with production cost: a ~40-byte format probe + /// becomes a whole-blob transfer, and it puts new offset arithmetic + /// on the read path, where a mistake serves wrong bytes silently + /// rather than failing. + /// + /// The real fix is the official `azure_storage_blob` 1.x, where + /// `range_get_content_crc64` is an explicit field on + /// `BlobClientDownloadOptions` — leave it unset and the request is + /// never made. Until then, the Azurite suite exercises enumeration + /// and round-trips but not migration; see + /// `tests/api/backend_consistency_azure.hurl`. fn get_blob_range_stream( &self, hash: &str, @@ -394,6 +449,197 @@ impl BlobStorageBackend for AzureBlobBackend { }) } + /// Enumerate blob hashes in lexicographic order, so + /// `backend_consistency` can merge-join against `storage.blobs` + /// instead of degrading to a per-row probe that structurally cannot + /// see orphans. + /// + /// ## Why this is a shard walk and not one flat listing + /// + /// **The cursor IS a blob hash**, not a provider token. The caller + /// forces that: it advances ONE cursor across both sides of the join, + /// feeding the same value here and to `WHERE hash > $1` in SQL. S3 + /// satisfies it with `start_after(object_key(cursor))`. + /// + /// Azure has no `StartAfter`. REST API 2023-05-03 added `startFrom`, + /// which would be the direct equivalent — but this SDK + /// (`azure_storage_blobs` 0.21, archived) never sends it: `ListBlobs` + /// exposes only `prefix`, `delimiter`, `max_results` and `marker`, + /// and `marker` is an opaque continuation token that cannot be + /// derived from a hash. + /// + /// So 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. Walking + /// `00/` … `ff/` therefore yields exactly the global hash order, and + /// a cursor names the shard to restart in. Re-listing on resume is + /// bounded by shard width — 1/256th of the container — rather than + /// by the whole container, which is what a client-side skip over a + /// flat listing would cost on every single page. + /// + /// `marker` is used only INSIDE one call, to page within a shard, and + /// never escapes as the cursor — the same treatment the S3 impl gives + /// its continuation token. + /// + /// ## What this does NOT see, unlike S3 + /// + /// S3 lists the bucket with no prefix, so any foreign object lands in + /// `unknowns`. Constraining to `{2-hex}/` means foreign names outside + /// that shape are invisible here. + /// + /// That asymmetry is deliberate and safe in the direction that + /// matters: an orphan is a blob **we** wrote and later stopped + /// referencing, so it always has the canonical name and is always + /// enumerated. Only genuinely foreign files — another workload + /// sharing the container — can be missed, and they are informational + /// notices, never findings. Trading them for O(N) enumeration instead + /// of O(N²/limit) is worth it. + fn list_blob_hashes( + &self, + cursor: Option, + limit: usize, + ) -> Pin> + Send + '_>> + { + Box::pin(async move { + // A run of foreign entries can't produce a resume cursor, and + // buffering the container to find one blob is worse than + // failing. Mirrors the S3 impl's bound, and like it is on + // entries accumulated rather than requests made: request + // count scales with the caller's `limit`, so a request cap + // would fire on a healthy container merely because the caller + // paged finely. + const MAX_UNKNOWNS: usize = 10_000; + + // A shard is `{2-hex}/`, so the space is 0x00..=0xff. + const LAST_SHARD: u16 = 0xff; + + let mut blobs: Vec = Vec::new(); + let mut unknowns: Vec = Vec::new(); + + // Resume in the cursor's own shard — its remaining entries + // still sort after it, and the client-side skip below drops + // the ones that don't. A malformed cursor is a bug in the + // caller's checkpoint, and silently restarting from `00` + // would re-report every blob as new, so refuse it. + let mut shard: u16 = match cursor.as_deref() { + Some(c) => u16::from(u8::from_str_radix(c.get(0..2).unwrap_or(""), 16).map_err( + |_| { + DomainError::internal_error( + "Blob", + format!( + "Azure enumeration cursor '{c}' is not a blob hash — it must \ + start with the two hex characters naming its shard" + ), + ) + }, + )?), + None => 0, + }; + + // Azure caps a page at 5000; asking for the caller's `limit` + // keeps a small page cheap. `MaxResults` rejects zero, and a + // caller asking for nothing still needs a well-formed + // request — and, more importantly, must not be answered with + // an empty page and a `None` cursor, which would read as + // "container fully enumerated, nothing here". + let want = limit.max(1); + let page_size = want.min(5000) as u32; + + 'shards: while shard <= LAST_SHARD { + let prefix = format!("{shard:02x}/"); + // `Pageable` follows `next_marker` itself, so one stream + // covers the whole shard however many round-trips it takes. + let mut pages = self + .container_client + .list_blobs() + .prefix(prefix) + .max_results(std::num::NonZeroU32::new(page_size).expect("clamped above 0")) + .into_stream(); + + while let Some(page) = pages.next().await { + let page = page.map_err(|e| { + DomainError::internal_error( + "Blob", + format!( + "Azure ListBlobs failed on shard {shard:02x} of container '{}': {e}", + self.container_name + ), + ) + })?; + + for blob in page.blobs.blobs() { + let name = blob.name.clone(); + // `OffsetDateTime` → chrono, for the caller's + // grace window. A value outside chrono's range + // degrades to `None`, which the port documents as + // "treat as old enough" — the conservative side, + // since it only ever suppresses a finding on a + // freshly-written blob. + let mtime = chrono::DateTime::::from_timestamp( + blob.properties.last_modified.unix_timestamp(), + blob.properties.last_modified.nanosecond(), + ); + + match Self::hash_from_blob_name(&name) { + Some(hash) => { + // `prefix` is inclusive of the cursor's own + // entry and of everything before it in the + // shard. Without this skip the caller sees + // a hash it already consumed and the + // merge-join never advances past it. + if cursor.as_deref().is_some_and(|c| hash.as_str() <= c) { + continue; + } + blobs.push(BackendBlobEntry { hash, mtime }); + } + // Not ours — a foreign workload sharing the + // container. Surfaced rather than dropped so + // an operator can see it. + None => unknowns.push(BackendUnknownEntry { path: name, mtime }), + } + } + + if blobs.len() >= want { + break 'shards; + } + + if unknowns.len() >= MAX_UNKNOWNS { + return Err(DomainError::internal_error( + "Blob", + format!( + "Azure enumeration accumulated {} non-blob entrie(s) without \ + filling a page, so no resume cursor can be produced. Container \ + '{}' likely holds a large foreign namespace — give OxiCloud a \ + dedicated container.", + unknowns.len(), + self.container_name, + ), + )); + } + } + + shard += 1; + } + + // Exhausting every shard is the ONLY end of enumeration. + // Stopping early because one shard was empty would truncate + // the sweep and report the rest of the container as absent, + // so `shard > LAST_SHARD` — not "this page was empty" — is + // what produces `None`. + let next_cursor = if shard > LAST_SHARD { + None + } else { + blobs.last().map(|entry| entry.hash.clone()) + }; + + Ok(BlobListPage { + blobs, + unknowns, + next_cursor, + }) + }) + } + fn backend_type(&self) -> &'static str { "azure" } @@ -406,14 +652,75 @@ impl BlobStorageBackend for AzureBlobBackend { fn local_blob_path(&self, _hash: &str) -> Option { None } - - // TODO: implement `list_blob_hashes` via - // `container_client.list_blobs()` (`azure_storage_blobs` - // paginator). Same filter as local + S3 impls: - // `/<64-hex>.blob` naming. Currently inherits the trait - // default which returns `operation_not_supported` — the - // `backend_consistency` tenant handles that by emitting a - // single run-level `backend_unenumerable` finding and - // completing without per-blob probes. Ship as a follow-up once - // there's an Azure test environment to validate against. +} + +#[cfg(test)] +mod tests { + use super::*; + + const H: &str = "0a1b2c3d4e5f60718293a4b5c6d7e8f90a1b2c3d4e5f60718293a4b5c6d7e8f9"; + + /// The enumeration cursor is fed straight back in as a shard prefix, + /// so a name that does not round-trip would resume in the wrong shard + /// and silently skip everything between. + #[test] + fn blob_name_round_trips_through_hash_from_blob_name() { + let name = AzureBlobBackend::blob_name(H); + assert_eq!(name, format!("0a/{H}.blob")); + assert_eq!( + AzureBlobBackend::hash_from_blob_name(&name).as_deref(), + Some(H) + ); + } + + /// Each of these would otherwise be treated as a hash — and the + /// resume path slices `[0..2]` off it to pick the next shard. + #[test] + fn non_canonical_names_are_rejected() { + let cases = [ + "0a/junk.tmp".to_string(), // spool file + "junk.tmp".to_string(), // no shard + "0a/junk".to_string(), // no suffix + "thumbnails/abc.jpg".to_string(), // foreign namespace + format!("0a/{H}.blob.corrupt"), // sidecar + format!("0a/{H}"), // suffix missing + format!("zz/{H}.blob"), // non-hex shard + format!("ff/{H}.blob"), // shard != hash prefix + format!("0a/{}.blob", &H[..63]), // wrong length + ]; + for name in &cases { + assert_eq!( + AzureBlobBackend::hash_from_blob_name(name), + None, + "must not be read as a blob: {name}" + ); + } + } + + /// The shard walk relies on `{hash[0..2]}/…` ordering lexicographic + /// names into exactly the order `ORDER BY hash` produces. If the + /// shard were not the hash's own prefix the two sequences would + /// interleave differently and the merge-join would emit phantom + /// findings in BOTH directions. + #[test] + fn shard_order_matches_hash_order() { + let hashes = ["00aa", "0a1b", "0aff", "b0cd", "ffff"] + .map(|p| format!("{p}{}", "0".repeat(60))) + .to_vec(); + + let mut names: Vec = hashes + .iter() + .map(|h| AzureBlobBackend::blob_name(h)) + .collect(); + names.sort(); + + let recovered: Vec = names + .iter() + .filter_map(|n| AzureBlobBackend::hash_from_blob_name(n)) + .collect(); + + let mut sorted_hashes = hashes.clone(); + sorted_hashes.sort(); + assert_eq!(recovered, sorted_hashes); + } } diff --git a/src/infrastructure/services/backend_consistency_service.rs b/src/infrastructure/services/backend_consistency_service.rs index 2b94f819..4ba04798 100644 --- a/src/infrastructure/services/backend_consistency_service.rs +++ b/src/infrastructure/services/backend_consistency_service.rs @@ -49,15 +49,38 @@ //! that ever admitted uppercase or variable length would break this //! silently and in both directions at once. //! -//! ### Run-level check +//! ### When enumeration fails //! -//! * `backend_unenumerable` (severity `anomaly`) — the backend -//! returned `operation_not_supported` on the first -//! `list_blob_hashes` call. Currently this fires when a -//! `MigrationBlobBackend` is active (refuses enumeration -//! mid-migration by design) or on an Azure backend (Azure impl -//! deferred). Informational — operators know they can't rely on -//! this scan under that config. +//! **The run fails.** There is no degraded mode. +//! +//! There used to be: an error on the first `list_blob_hashes` call +//! emitted a `backend_unenumerable` anomaly and fell back to +//! `probe_each_row`, one `blob_exists` per `storage.blobs` row. That +//! recovered `blob_missing_from_backend` (the direction that loses +//! FILES) but never `orphan_blob`, since bytes no row claims are +//! invisible to anything starting from the database. +//! +//! It was written for two cases, and neither exists: +//! +//! * **Azure** — enumerates since the 256-way shard walk (see +//! `AzureBlobBackend::list_blob_hashes` for why it needs one to do +//! what S3 gets from `StartAfter`). +//! * **Mid-migration** — never applied. That justification named a +//! `MigrationBlobBackend` that does not exist; +//! `SwappableBlobBackend::list_blob_hashes` forwards to whatever is +//! currently active, as do the Encrypted, Cached and Retry wrappers. +//! Do not reintroduce the claim without grepping for the impl. +//! +//! So the only thing still reaching it was a *transient* failure — auth +//! blip, throttle, network — being relabelled as a capability limit and +//! silently costing orphan coverage. A failed run is louder than an +//! anomaly on an otherwise-clean-looking scan, which was the fallback's +//! own stated goal. +//! +//! The trait default still returns `operation_not_supported`, so a +//! future write-only or read-only-mirror backend would fail every run +//! here. **That is when the fallback should come back — with tests.** +//! It had none, which is the other half of why it went. //! //! ### Grace window //! @@ -116,10 +139,6 @@ const BATCH_SIZE: usize = 500; /// `blobs_consistency` + `dedup_gc`. const CREATE_GRACE: Duration = Duration::hours(1); -/// Cap on affected-blob examples surfaced in the run-level -/// `backend_unenumerable` finding. Keeps the finding detail bounded. -const _MAX_EXAMPLES: usize = 5; - pub struct BackendConsistencyCheck { pool: Arc, /// Default backend to enumerate when `args.storage` is `None` — @@ -172,10 +191,12 @@ impl RecoverableJobHandler for BackendConsistencyCheck { "Merge-joins the storage backend's blob enumeration against \ storage.blobs, both ordered by hash, so one pass yields the delta \ in both directions: bytes on the backend no DB row claims, and \ - rows whose bytes are gone. Add ?deep=true to also read every \ - matched blob back and re-hash it, catching silent bit-rot — that \ - is a full read of storage and can take hours. Read-only in both \ - modes: nothing is uploaded or deleted." + rows whose bytes are gone. If the backend cannot be enumerated \ + the run fails rather than reporting partial coverage. \ + Add ?deep=true to also read every matched blob back and re-hash \ + it, catching silent bit-rot — that is a full read of storage and \ + can take hours. Read-only in every mode: nothing is uploaded or \ + deleted." } /// Approximate total: on a healthy install every backend blob @@ -377,45 +398,25 @@ impl RecoverableJobHandler for BackendConsistencyCheck { let page = match backend.list_blob_hashes(cursor.clone(), BATCH_SIZE).await { Ok(v) => v, Err(e) => { - // Backend refuses / can't enumerate. First-batch - // failure = we emit ONE run-level anomaly and - // complete cleanly (the run stays useful — the - // operator learns why nothing was checked - // instead of getting a red error). Mid-scan - // failure = we fail the run. - - let is_first_batch = cursor.is_none() && finding_count == 0; - if is_first_batch { - // No local increment — the local - // `finding_count` is only used for the - // completion log below, but this branch - // returns immediately. The finding IS - // persisted + counted in `stats.finding_count` - // by `record_or_log` → `store.record_finding`. - record_or_log( - store, - BACKEND_CONSISTENCY_JOB_NAME, - "backend_unenumerable", - "anomaly", - None, - serde_json::json!({ - "backend": backend.backend_type(), - "error": format!("{e}"), - "note": "backend refused enumeration; no per-blob orphan probes attempted", - }), - ) - .await; - tracing::info!( - target: "oxicloud::consistency", - event = "backend_consistency.unenumerable", - run_id = %store.run_id(), - backend = backend.backend_type(), - "backend refused enumeration (typical during migration or on backends without list support)" - ); - return RunOutcome::completed(); - } + // Fail loudly, first batch or not. + // + // A first-batch failure used to degrade to + // `probe_each_row` instead. That was written for + // backends which genuinely cannot enumerate, and none + // ship today — see the module docs for why the two it + // named do not apply. What was left reaching it was a + // transient error relabelled as a capability limit, on + // a run that then looked clean while having lost orphan + // coverage entirely. + // + // Whether the enumeration died on page 1 or page 900, + // the audit did not complete, and the operator needs to + // know that rather than read a green run. return RunOutcome::Failed { - message: format!("backend list failed mid-scan: {e}"), + message: format!( + "backend enumeration failed on {}: {e}", + backend.backend_type() + ), }; } };