refactor(core): shared single-flight primitive; fix dunite-download lossy error mirror (PSA-36) #5

Merged

nrupard merged 2 commits from refactor/psa-36-single-flight-core into main

2026-06-02 20:27:35 +02:00

nrupard commented

2026-06-02 20:13:04 +02:00

Owner

What

Implements PSA-36: the follow-up from PSA-35's code review.

1. Shared single-flight primitive in dunite-core

New dunite_core::services::single_flight::SingleFlight<K, T, E>: a keyed single-flight executor (map + OnceCell + insert/remove locking). The first caller for a key runs the operation; concurrent callers await the same cell and receive a clone of the result; the key is removed on completion so failures are never cached. Both vertical caches (dunite-oci blob_cache, dunite-download download_cache) now use it instead of each hand-rolling the same map/cell/locking pattern (architecture rule: cross-vertical mechanism goes DOWN into core).

2. dunite-download lossy-mirror bug fix (same bug PSA-35 fixed in dunite-oci)

ForgejoError::Http (Forgejo unreachable: connection refused, DNS, TLS, timeout) collapsed into the mirror's Other(String) catch-all and round-tripped back as DownloadCacheError::Io, so a binary download against a down Forgejo was misclassified as a local filesystem error (HTTP 500) instead of an upstream failure (502). The mirror is now total in both directions: new public DownloadCacheError::Transport variant, dedicated Io/Store arms, no catch-all. Mid-stream body errors are also Transport now. The compiler now enforces totality: a future variant added to either error enum is a compile error until its classification is chosen.

3. Cleanups from the PSA-35 review

dunite-oci/tests/limiter.rs deduped: engine behavior is covered by dunite-core's own tests; only the slot-leak regression test (consumer-facing API surface) remains.
Limiter aliasing convention documented in dunite-download's lib.rs: core names (LimitDenial) are canonical; dunite-oci's full aliasing is backward compatibility for consumers written against its pre-PSA-35 API. Renaming was rejected because bunyip matches LimitDenial::Concurrency/DailyCap directly and a rename breaks consumers for cosmetics.
CLAUDE.md documents single_flight and the total-mirror rule.

Tests

122 tests green in the rust-builder container (fmt + clippy -D warnings clean):

dunite-core: 64 (+3 new single_flight tests: leader/waiter sharing, failures-not-cached, key independence)
dunite-download: 27 (+1 new upstream_unreachable_surfaces_as_transport_not_io regression test)
dunite-oci: 27 + 1 integration
dunite-oidc: 3

Consumer impact

None breaking. bunyip's download handler uses matches!() on specific variants (not exhaustive match), so the new Transport variant flows into its existing generic upstream path (HTTP 502) - the correct classification. bunyip picks up the improvements on its next cargo update.

## What Implements PSA-36: the follow-up from PSA-35's code review. ### 1. Shared single-flight primitive in dunite-core New `dunite_core::services::single_flight::SingleFlight<K, T, E>`: a keyed single-flight executor (map + `OnceCell` + insert/remove locking). The first caller for a key runs the operation; concurrent callers await the same cell and receive a clone of the result; the key is removed on completion so failures are never cached. Both vertical caches (dunite-oci `blob_cache`, dunite-download `download_cache`) now use it instead of each hand-rolling the same map/cell/locking pattern (architecture rule: cross-vertical mechanism goes DOWN into core). ### 2. dunite-download lossy-mirror bug fix (same bug PSA-35 fixed in dunite-oci) `ForgejoError::Http` (Forgejo unreachable: connection refused, DNS, TLS, timeout) collapsed into the mirror's `Other(String)` catch-all and round-tripped back as `DownloadCacheError::Io`, so a binary download against a down Forgejo was misclassified as a local filesystem error (HTTP 500) instead of an upstream failure (502). The mirror is now total in both directions: new public `DownloadCacheError::Transport` variant, dedicated `Io`/`Store` arms, no catch-all. Mid-stream body errors are also `Transport` now. The compiler now enforces totality: a future variant added to either error enum is a compile error until its classification is chosen. ### 3. Cleanups from the PSA-35 review - `dunite-oci/tests/limiter.rs` deduped: engine behavior is covered by dunite-core's own tests; only the slot-leak regression test (consumer-facing API surface) remains. - Limiter aliasing convention documented in dunite-download's `lib.rs`: core names (`LimitDenial`) are canonical; dunite-oci's full aliasing is backward compatibility for consumers written against its pre-PSA-35 API. Renaming was rejected because bunyip matches `LimitDenial::Concurrency`/`DailyCap` directly and a rename breaks consumers for cosmetics. - CLAUDE.md documents `single_flight` and the total-mirror rule. ## Tests 122 tests green in the rust-builder container (fmt + clippy -D warnings clean): - dunite-core: 64 (+3 new single_flight tests: leader/waiter sharing, failures-not-cached, key independence) - dunite-download: 27 (+1 new `upstream_unreachable_surfaces_as_transport_not_io` regression test) - dunite-oci: 27 + 1 integration - dunite-oidc: 3 ## Consumer impact None breaking. bunyip's download handler uses `matches!()` on specific variants (not exhaustive match), so the new `Transport` variant flows into its existing generic upstream path (HTTP 502) - the correct classification. bunyip picks up the improvements on its next `cargo update`.

nrupard added 1 commit

2026-06-02 20:13:04 +02:00

refactor(core): add shared single-flight primitive; fix dunite-download's lossy error mirror (PSA-36)

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

Details

5776ade7eb

- New dunite_core::services::single_flight::SingleFlight<K, T, E>: keyed single-flight executor (map + OnceCell + insert/remove locking) shared by both vertical caches, so the mechanism lives in one place instead of being hand-duplicated. Failures are never cached; concurrent callers for a key share the leader's result.
- Fix dunite-download's FetchError mirror, the same lossy-catch-all bug PSA-35 fixed in dunite-oci: ForgejoError::Http (Forgejo unreachable: connection refused, DNS, TLS, timeout) collapsed into Other(String) and round-tripped back as DownloadCacheError::Io, misclassifying upstream outages as local filesystem errors. The mirror is now total in both directions with a new public DownloadCacheError::Transport variant and dedicated Io/Store arms; mid-stream body errors are also Transport now. Regression test added (upstream_unreachable_surfaces_as_transport_not_io).
- Both caches (dunite-oci blob_cache, dunite-download download_cache) now run fetches through the core SingleFlight; their per-vertical mirrors stay but must remain total - no catch-all arms - which the compiler now enforces.
- Dedupe dunite-oci/tests/limiter.rs: engine behavior (guard drop, daily-cap rollback) is covered by dunite-core's own tests; only the slot-leak regression test that pins the consumer-facing re-exported API remains.
- Document the limiter aliasing convention in dunite-download's lib.rs (core names are canonical; dunite-oci's aliases are backward compatibility) and the single_flight/total-mirror rules in CLAUDE.md.

Consumer impact: none breaking. bunyip's download handler uses matches!() on specific DownloadCacheError variants, so the new Transport variant flows into its existing generic upstream path (HTTP 502), which is the correct classification.

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

nrupard added 1 commit

2026-06-02 20:23:43 +02:00

fix(core): guard single-flight removal with cell identity; narrow tokio features (PSA-36 review)

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

Details

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

Details

330bf5c3f5

Code-review findings on the single-flight primitive:

- SingleFlight::run removed the key unconditionally after completion, so a lagging waiter from an earlier cohort could delete a NEWER cohort's in-flight cell, allowing two concurrent operations for the same key (for the OCI blob cache both would write the same .partial path). Removal now happens through a RemoveOnDrop guard that only removes the entry while the map still holds the SAME cell (Arc::ptr_eq). The guard also runs on panic, so a panicking operation cannot leak a map entry. New in_flight() diagnostic method and regression tests for both properties.
- The workspace tokio baseline is narrowed to ["sync"] (all dunite-core needs for OnceCell); the vertical crates additively enable "full" for their fs/io/spawn use, and dunite-core's tests get rt/macros/time as dev-dependencies. The foundation crate no longer advertises the full tokio runtime to consumers.
- Module doc now records why moka was not used (moka coalesces but caches; this primitive coalesces without caching) and that mirrors are classification-preserving, not identity-preserving. Both verticals' mirror docs point at the single_flight module doc as the canonical statement of the rules instead of restating them.

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