refactor(oci): migrate dunite-oci to shared core limiter, origin check, typed blob errors (PSA-35) #4

Merged

nrupard merged 2 commits from refactor/psa-35-oci-shared-core into main

2026-06-02 19:55:03 +02:00

nrupard commented

2026-06-02 19:27:15 +02:00

Owner

What

Implements PSA-35: dunite-oci now builds on dunite-core's shared mechanisms instead of carrying near-identical copies.

Limiter: OciLimiter / OciPullGuard / OciLimitDenial are now re-exports of dunite_core::services::usage_limiter::{UsageLimiter, UsageGuard, LimitDenial}. The duplicated implementation in oci_limiter.rs is deleted, and with it the slot-leak rollback bug (the old copy decremented the daily count BEFORE releasing the in-flight slot and swallowed decrement errors, so a failing decrement leaked the concurrency slot permanently).
PullCounter: now a re-export of core's UsageCounter, same pattern as dunite-download's DownloadCounter. The current() method is dropped; nothing in the engine called it.
Origin check: forgejo_registry::validate_host delegates to dunite_core::validation::origin::same_origin.
Typed blob errors (scope added via issue comment, BUNYIP-31 finding): the blob cache's single-flight path now uses BlobCacheError + a cloneable FetchError mirror (the dunite-download pattern). Upstream 404 surfaces as Registry(NotFound), integrity violations as DigestMismatch, filesystem failures as Io, instead of everything flattening into an opaque AppError::internal string. A permission-denied cache dir is now diagnosable from consumer logs.

Tests

New tests/limiter.rs pins the re-exported API surface and adds a regression test: a failing daily-count decrement can no longer leak a concurrency slot (failing_decrement_does_not_leak_concurrency_slot).
Blob cache tests updated to assert typed variants; new filesystem_failure_surfaces_as_typed_io_error test.
Full workspace green in the rust-builder container: 118 passed, 0 failed (fmt + clippy -D warnings also clean).

Consumer impact (bunyip-oci, BUNYIP-26)

Public API changes, coordinated as the PSA-35 consumer-side follow-up:

PullCounter impl: remove current() from the trait impl block (keep it as an inherent method if the repository's own tests use it).
get_blob handler: match on BlobCacheError::{Registry, InvalidDigest, DigestMismatch, Io, Store} instead of AppError variants. The handler comment already tracks this (PSA-35 reference).
BlobCache::ensure_dir / evict_if_over_cap / sweep_orphans return types changed from io::Error / AppError to BlobCacheError.

bunyip-oci pins branch = "main", so these land there on its next cargo update after merge.

## What Implements PSA-35: dunite-oci now builds on dunite-core's shared mechanisms instead of carrying near-identical copies. - **Limiter**: `OciLimiter` / `OciPullGuard` / `OciLimitDenial` are now re-exports of `dunite_core::services::usage_limiter::{UsageLimiter, UsageGuard, LimitDenial}`. The duplicated implementation in `oci_limiter.rs` is deleted, and with it the slot-leak rollback bug (the old copy decremented the daily count BEFORE releasing the in-flight slot and swallowed decrement errors, so a failing decrement leaked the concurrency slot permanently). - **PullCounter**: now a re-export of core's `UsageCounter`, same pattern as dunite-download's `DownloadCounter`. The `current()` method is dropped; nothing in the engine called it. - **Origin check**: `forgejo_registry::validate_host` delegates to `dunite_core::validation::origin::same_origin`. - **Typed blob errors** (scope added via issue comment, BUNYIP-31 finding): the blob cache's single-flight path now uses `BlobCacheError` + a cloneable `FetchError` mirror (the dunite-download pattern). Upstream 404 surfaces as `Registry(NotFound)`, integrity violations as `DigestMismatch`, filesystem failures as `Io`, instead of everything flattening into an opaque `AppError::internal` string. A permission-denied cache dir is now diagnosable from consumer logs. ## Tests - New `tests/limiter.rs` pins the re-exported API surface and adds a regression test: a failing daily-count decrement can no longer leak a concurrency slot (`failing_decrement_does_not_leak_concurrency_slot`). - Blob cache tests updated to assert typed variants; new `filesystem_failure_surfaces_as_typed_io_error` test. - Full workspace green in the rust-builder container: 118 passed, 0 failed (fmt + clippy -D warnings also clean). ## Consumer impact (bunyip-oci, BUNYIP-26) Public API changes, coordinated as the PSA-35 consumer-side follow-up: - `PullCounter` impl: remove `current()` from the trait impl block (keep it as an inherent method if the repository's own tests use it). - `get_blob` handler: match on `BlobCacheError::{Registry, InvalidDigest, DigestMismatch, Io, Store}` instead of `AppError` variants. The handler comment already tracks this (PSA-35 reference). - `BlobCache::ensure_dir` / `evict_if_over_cap` / `sweep_orphans` return types changed from `io::Error` / `AppError` to `BlobCacheError`. bunyip-oci pins `branch = "main"`, so these land there on its next `cargo update` after merge.

nrupard added 1 commit

2026-06-02 19:27:15 +02:00

refactor(oci): migrate dunite-oci to shared core limiter, origin check, and typed blob errors (PSA-35)

Checks / fmt + clippy + test (pull_request) Successful in 1m28s

Details

217d13f99c

- Replace OciLimiter / OciPullGuard / OciLimitDenial with re-exports of dunite-core's UsageLimiter / UsageGuard / LimitDenial. This removes the duplicated limiter implementation and with it the slot-leak rollback bug: the old copy decremented the daily count before releasing the in-flight slot and swallowed decrement errors, so a failing decrement leaked the concurrency slot permanently.
- Replace the PullCounter trait with a re-export of dunite-core's UsageCounter (same pattern as dunite-download's DownloadCounter). The current() method is dropped: nothing in the engine calls it, and consumers can keep an inherent method on their repository if they need one.
- forgejo_registry::validate_host now delegates to dunite_core::validation::origin::same_origin instead of carrying its own scheme/host/port comparison.
- Migrate the blob cache to typed errors (BlobCacheError + cloneable FetchError mirror, the dunite-download pattern). Upstream 404s now surface as Registry(NotFound), integrity violations as DigestMismatch, and filesystem failures (permission denied, disk full) as Io, instead of all being flattened into an opaque internal string. This was the BUNYIP-31 finding where a permission-denied cache dir surfaced to docker as a generic 502 with no diagnosable log.
- Add tests/limiter.rs pinning the re-exported limiter API surface and a regression test proving a failing decrement can no longer leak a concurrency slot.
- Update CLAUDE.md's architecture rule to reflect that both verticals now share the core limiter and counter trait.

Consumer impact (bunyip-oci, tracked in PSA-35): the PullCounter impl must drop its current() trait method (keep it as an inherent method), and the get_blob handler maps BlobCacheError variants instead of AppError. Coordinated as the consumer-side follow-up.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

nrupard added 1 commit

2026-06-02 19:50:33 +02:00

fix(oci): make the blob-cache error mirror total; classify OCI errors in-crate (PSA-35 review)

Checks / fmt + clippy + test (pull_request) Successful in 23s

Details

create-release / create-release (pull_request) Has been skipped

Details

8decf5c21a

Code-review findings on the typed-error migration:

- The FetchError mirror had a lossy Other(String) catch-all: RegistryError::Http (Forgejo unreachable: connection refused, DNS, TLS, timeout), RegistryError::UrlParse, Io, and Store errors all collapsed into Other and round-tripped back as BlobCacheError::Io. A docker pull against a down Forgejo would surface as HTTP 500 with a "filesystem cause" log instead of 502 Upstream. The mirror is now total in both directions with dedicated Transport/Io/Store variants and no catch-all; a new public BlobCacheError::Transport variant carries transport failures whose original error cannot be cloned through the single-flight cell.
- Mid-stream body errors (connection dropped while streaming a blob) are now Transport, not Io, for the same reason.
- The single-flight cell now carries the fetched blob's metadata (media type + size) instead of (), so get_or_fetch builds the BlobHandle directly: removes one store.find() round-trip per fetch and the defensive "row missing after fetch" branch entirely.
- validate_digest is called once (in get_or_fetch) instead of twice; fetch_and_store documents the precondition.
- New From<&BlobCacheError> for OciError in dunite-oci: the OCI-spec status classification (BlobUnknown / Upstream / Internal) now lives next to both types so every consumer shares it instead of re-deriving the match; consumers keep only their logging/audit policy.
- New tests: upstream_unreachable_surfaces_as_transport_not_io (regression for the lossy mirror) and oci_error_classification_covers_every_variant.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>