From 992bdae8983fa40394eeaa3f8dafb83b835a6f14 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 13:48:47 +0000 Subject: [PATCH 1/3] =?UTF-8?q?perf:=20round=2022=20=E2=80=94=20hot-GET=20?= =?UTF-8?q?HeaderMap=20borrow,=20native-WebDAV/CalDAV=20etag=20borrowed=20?= =?UTF-8?q?quotes,=20FileDto=20content=5Fhash=20move,=20CalendarEvent=20st?= =?UTF-8?q?amp,=20ShareItemType=20case-fold?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Benchmark-gated, same rule as ROUND2-21: every change ships with a BEFORE/AFTER counting-allocator benchmark and a byte/-value equivalence gate; an AFTER that fails to reduce allocations exits non-zero (rollback). See benches/ROUND22.md and examples/bench_round22_micro.rs. All arms no-Postgres. - H1: the hot GET handlers (get_thumbnail, download_file, list_files_query, list_photos, NextCloud preview, public-share download/access) take `req: Request` last and read `req.headers()` by borrow instead of axum's HeaderMap extractor, whose FromRequestParts impl clones the whole request header table just to read 1-3 headers (the ROUND14 §A4 middleware pattern, finally propagated to the handlers). 2 -> 0 allocs/req · 9.95x wall. - W1: native WebDAV write_etag_quoted — the etag emitter for every /webdav/ PROPFIND row (per file AND per folder, up to 500/page) — emits the quotes as borrowed pre-escaped " text events instead of escaping a "{etag}" String (the ROUND20 §C1 / ROUND21 §R4 pattern). 3 -> 0 allocs/row. - C1: CalDAV getetag routed through a shared write_quoted_etag helper across all 5 sites (3 per-event + 2 per-calendar); the now-dead etag: &mut String buffer threaded through write_event_response/standard/requested props + the two per-page buffers removed. 2 -> 0 allocs/row. - D1: FileDto::from reuses the moved parts.blob_hash instead of cloning it via the content_hash() getter (the ROUND19/20 move-not-clone sweep missed it — hash/etag are read before into_parts()). Per file row of every listing. 1 -> 0 allocs/row. - E1: CalendarEvent::update_time_range/update_all_day stamp timed DTSTART/DTEND via fmt::compact_ical_utc stack render (chrono fallback out of range) instead of the %Y%m%dT%H%M%SZ strftime interpreter. 4 -> 0. - S1: ShareItemType::try_from uses eq_ignore_ascii_case instead of a throwaway to_lowercase() String. 1 -> 0 allocs/parse. Verified: cargo clippy --features bench --all-targets -D warnings clean, cargo fmt --all --check clean, cargo test --lib --features bench = 529 passed / 0 failed (incl. the OpenAPI-spec-validity test guarding the H1 utoipa-handler signature change). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01DKyQ4AnYtgp1JtjzweyMeo --- Cargo.toml | 16 + benches/ROUND22.md | 216 ++++++++ examples/bench_round22_micro.rs | 521 ++++++++++++++++++ src/application/adapters/caldav_adapter.rs | 74 +-- src/application/adapters/webdav_adapter.rs | 17 +- src/application/dtos/file_dto.rs | 11 +- src/domain/entities/calendar_event.rs | 81 ++- src/domain/entities/share.rs | 15 +- src/interfaces/api/handlers/file_handler.rs | 36 +- src/interfaces/api/handlers/photos_handler.rs | 9 +- src/interfaces/api/handlers/share_handler.rs | 15 +- src/interfaces/nextcloud/preview_handler.rs | 6 +- 12 files changed, 918 insertions(+), 99 deletions(-) create mode 100644 benches/ROUND22.md create mode 100644 examples/bench_round22_micro.rs diff --git a/Cargo.toml b/Cargo.toml index 0581b5a8..a678a0c0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -354,6 +354,22 @@ name = "bench_micro_allocs" path = "examples/bench_micro_allocs.rs" required-features = ["bench"] +# Round-22 battery ──────────────────────────────────────────────────────────── + +# Round-22 CPU/alloc micro-pack — the deferred hot-GET-handler `HeaderMap` +# extractor clone (thumbnail/list/download/photos/preview/share read 1–3 headers +# out of a full `parts.headers.clone()`), routed through `Request` + borrow (H1); +# native-WebDAV `write_etag_quoted` borrowed pre-escaped quotes, the sweep's last +# per-row DAV site (W1); CalDAV `getetag` shared `write_quoted_etag` helper across +# 5 sites + dead etag-buffer removal (C1); `FileDto::from` reuse of the moved +# `blob_hash` instead of the getter clone the move-not-clone sweep missed (D1); +# `CalendarEvent` timed DTSTART/DTEND via `fmt::compact_ical_utc` (E1); +# `ShareItemType::try_from` `eq_ignore_ascii_case` (S1). No Postgres. +[[example]] +name = "bench_round22_micro" +path = "examples/bench_round22_micro.rs" +required-features = ["bench"] + # Round-21 battery ──────────────────────────────────────────────────────────── # Round-21 CPU/alloc micro-pack — CalDAV/CardDAV row-mapper container pre-size diff --git a/benches/ROUND22.md b/benches/ROUND22.md new file mode 100644 index 00000000..750ea615 --- /dev/null +++ b/benches/ROUND22.md @@ -0,0 +1,216 @@ +# Round 22 — hot-GET HeaderMap borrow, native-WebDAV & CalDAV etag borrowed quotes, FileDto content_hash move, CalendarEvent stamp, ShareItemType case-fold + +Benchmark-gated, same rule as ROUND2–21: every change ships with a BEFORE/AFTER +benchmark and a byte/-value equivalence gate; an AFTER that doesn't beat its +BEFORE is rolled back (never applied). The roll-back rule is encoded directly in +the harness — a `GATE FAIL … rollback` non-zero exit if an AFTER arm fails to +reduce allocations — so a regression fails CI rather than shipping. + +This round drains the two biggest items the ROUND21 audit explicitly deferred — +the hot-GET-handler `HeaderMap` extractor clone and the two DAV etag emitters the +borrowed-pre-escaped-quote sweep never reached (native WebDAV + CalDAV) — plus +the `FileDto::from` `content_hash` clone the ROUND19/20 move-not-clone sweep +missed (it is computed *before* `into_parts()`), and two low-heat strftime / +case-fold cuts. + +Reproduce: + +``` +cargo run --release --features bench --example bench_round22_micro +``` + +All arms are **no-Postgres** (release-profile counting-allocator example). + +## Summary + +| # | change | key metric | before → after | +|--:|---|---|---| +| **H1** | The hot GET handlers (`get_thumbnail`, `download_file`, `list_files_query`, `list_photos`, NextCloud `preview`, public-share `download`/`access`) took axum's `HeaderMap` extractor, whose `FromRequestParts` impl does `parts.headers.clone()` — an owned clone of the **whole** request header table — purely to read 1–3 headers (`If-None-Match` / `Accept` / `Range` / unlock cookie). Now they take `req: Request` last and read `req.headers()` by borrow (the ROUND14 §A4 middleware pattern, finally propagated to the handlers). The 3 wrapper/`_impl` file handlers pass `req.headers()` into an `_impl` that now takes `&HeaderMap` (`+ use<>` on the return so the 2024-edition `impl Trait` capture doesn't tie the owned `Response` to the borrow). | realistic 13-header req | **2 → 0 allocs/op · 9.95× wall** | +| **W1** | `webdav_adapter::write_etag_quoted` — the etag emitter for **every** native `/webdav/` PROPFIND row (per file AND per folder, up to `PROPFIND_BATCH_SIZE`=500/page — the most-travelled DAV path) — built a sized `"{etag}"` `String` then wrote it auto-escaped; `quick_xml` escapes the `"` → `"`, re-allocating an owned `Cow`. Now emits the two quotes as borrowed pre-escaped `"` text events around the escaped body (the ROUND20 §C1 / ROUND21 §R4 pattern the native adapter never got). Byte-identical for any etag. | per PROPFIND row | **3 → 0 allocs/op · 1.59× wall** | +| **C1** | The CalDAV `getetag` emit — per event of every calendar-query/multiget/sync REPORT + depth-1 collection PROPFIND (the DAVx5/Apple/Thunderbird sync path), and per calendar of the home-set PROPFIND — still escaped a `"…"` value: the event sites paid the escape `Cow` over the ROUND14 reused buffer (1 alloc/event); the two calendar sites `format!`-ed as well (2 allocs). All **five** sites now route through a new `write_quoted_etag` helper (the CardDAV twin), and the now-dead `etag: &mut String` buffer threaded through `write_event_response`/`write_event_standard_props`/`write_event_requested_props` + the two page buffers are dropped. | per event row | **2 → 0 allocs/op · 1.63× wall** | +| **D1** | `FileDto::from` computed `content_hash = file.content_hash().to_string()` (a clone of `blob_hash`) and then `into_parts()` **moved** that same `blob_hash` into `parts.blob_hash`, which was dropped unused in the `Self { … }` ctor. The ROUND19/20 move-not-clone sweep fixed id/name/path/folder_id but missed this one because the etag/hash are read *before* `into_parts()`. Now `content_hash: parts.blob_hash` reuses the moved `String`; `etag` still computes first from the live entity. Runs **per file row on every listing** (folder browse, streaming PROPFIND, search/favorites/recent hydration). | per file row | **1 → 0 allocs/op · 2.91× wall** | +| **E1** | `CalendarEvent::update_time_range` / `update_all_day` stamped **timed** DTSTART/DTEND via `format!("{}", t.format("%Y%m%dT%H%M%SZ"))` — chrono's strftime `DelayedFormat` interpreter. Now stack-renders via the shipped `fmt::compact_ical_utc` and passes the `&str` straight to `update_ical_property`, with the chrono `format!` kept as the out-of-range fallback and the all-day `%Y%m%d` form untouched. Per event-edit PUT. | per timed stamp | **4 → 0 allocs/op · 14.49× wall** | +| **S1** | `ShareItemType::try_from` matched `s.to_lowercase().as_str()` — a throwaway Unicode-lowercased `String` — against the two ASCII literals `"file"`/`"folder"`. Now `s.eq_ignore_ascii_case("file")` / `("folder")`: byte-identical acceptance for the ASCII targets, no allocation. | per parse | **1 → 0 allocs/op · 7.81× wall** | + +> Allocs/op is the deterministic primary gate (identical run to run). Wall +> figures are single-shot and noise-bounded. Every section carries a +> byte/-value equivalence gate; the shipped source now matches each AFTER arm. + +## [H1] Hot GET handler `HeaderMap` extractor → `Request` + borrow + +axum 0.8's `impl FromRequestParts for HeaderMap` is literally +`Ok(parts.headers.clone())` — cloning the whole request header table (its +`entries` + `indices` backing vectors; the counting allocator measures exactly +2 allocs on a realistic 13-header browser request). The handlers below read only +1–3 headers out of it, so the clone is pure waste — the exact cost +`middleware/auth.rs` removed in ROUND14 §A4 (`request.headers().get(…)` by +borrow) but which was never propagated to the handlers. + +The fix takes `req: Request` as the **last** extractor (all the others — +`State`, `AuthUser`, `Path`, `Query` — are `FromRequestParts`, so they coexist +with a single trailing `FromRequest`), and reads `req.headers()` by borrow: + +- **Standalone handlers** (`list_photos`, NC `preview`, share `download`/`access`): + swap `headers: HeaderMap` for `req: Request` and read `req.headers().get(…)` + at the (single) use site. +- **Wrapper/`_impl` handlers** (`get_thumbnail`, `download_file`, + `list_files_query`): the wrapper takes `req: Request` and passes + `req.headers()` into an `_impl` whose param becomes `headers: &HeaderMap`. The + `_impl` return type gets `+ use<>` so the 2024-edition `impl Trait` lifetime + capture doesn't tie the (owned) `Response` output to the header borrow — the + future still borrows the headers during its inline `.await`, but the response + it yields captures nothing. + +Byte-identical: every call site reads the same header by `.get()`. The +`openapi_spec_is_valid_and_has_expected_structure` test confirms the +utoipa-annotated handlers still emit a valid spec after the signature change. + +NextCloud `avatar` (dual caller `handle_dav_avatar` → `handle_avatar` + dual +route) and the share-management handlers (`create`/`update`/… take a `Json` +body, so no second `Request` extractor is possible) were left for a dedicated +pass — see *Not shipped*. + +## [W1] Native WebDAV `getetag` — borrowed pre-escaped quotes + +`write_etag_quoted` is the single helper behind all four native PROPFIND etag +sites (`webdav_adapter.rs:857/935/1004/1089` — file + folder, allprop + named). +It built a `String::with_capacity(etag.len()+2)` `"{etag}"` and wrote it via +`BytesText::new`, which escapes the `"` → `"` and re-allocates an owned +`Cow`. Now (the ROUND20 §C1 / ROUND21 §R4 shape): + +```rust +xml_writer.write_event(Event::Text(BytesText::from_escaped(""")))?; // borrowed +xml_writer.write_event(Event::Text(BytesText::new(etag)))?; // escaped body +xml_writer.write_event(Event::Text(BytesText::from_escaped(""")))?; +``` + +`escape` maps `"`→`"` per char, so `"{escape(etag)}"` is +byte-identical to escaping `"{etag}"` for **any** etag (the equivalence gate +asserts it, including an etag carrying `&`/`<`/`"`). One helper body fixes all +four call sites — 0 allocs/row on the hottest native-WebDAV path. + +## [C1] CalDAV `getetag` — shared `write_quoted_etag` helper (5 sites) + +The CalDAV adapter was the last DAV emitter still escaping a quoted etag value. +A new file-local `write_quoted_etag` (identical to the shipped CardDAV twin) +replaces the manual quote-and-escape at all five sites: + +- `write_event_standard_props` / `write_event_requested_props` / + `write_collection_event_page` — **per event bundle** (the reused ROUND14 + buffer was already amortized, so the remaining cost was the escape `Cow`; + 1 → 0 alloc/event). +- `write_calendar_standard_props` / `write_calendar_requested_props` — **per + calendar**, which additionally `format!`-ed the value (2 → 0). + +The etag bodies are bare `Uuid`s (`anchor.id` / `calendar.id`), so +`BytesText::new(id)` is itself a borrow (0 allocs). With the emit no longer +needing a scratch `String`, the `etag: &mut String` buffer threaded through +`write_event_response` → `write_event_standard_props` / +`write_event_requested_props` and the two per-page `String::new()` buffers were +removed. The 34 caldav-adapter unit tests (PROPFIND/REPORT output) pass +unchanged. + +## [D1] `FileDto::from` — reuse the moved `blob_hash`, don't clone it + +The per-row DTO builder computed the ETag and the content hash from the live +entity, then consumed it: + +```rust +let etag = file.etag(); +let content_hash = file.content_hash().to_string(); // clone of self.blob_hash +let parts = file.into_parts(); // MOVES self.blob_hash → parts.blob_hash +// … Self { …, content_hash, etag, … } // parts.blob_hash dropped unused +``` + +`etag` genuinely must run against the live entity (it borrows `blob_hash` + +`modified_at`), but `content_hash` is just the raw hash — and `into_parts()` +already hands it over by ownership. Now `content_hash: parts.blob_hash` reuses +that `String`; the getter clone (one 64-byte hex `String` per row) is gone. This +is the file-side twin of the fields `FolderDto::from` already moves, on the +single most-travelled API path. Byte-identical: `parts.blob_hash` **is** the +`String` the getter cloned. + +## [E1] `CalendarEvent` timed DTSTART/DTEND — `compact_ical_utc` stack render + +The timed branches of `update_time_range` / `update_all_day` stamped +`format!("{}", t.format("%Y%m%dT%H%M%SZ"))`, running chrono's strftime +interpreter (4 allocs measured). `fmt::compact_ical_utc` already renders exactly +`YYYYMMDDTHHMMSSZ` on the stack (the ROUND19 §V2 helper), and the property +setter takes a `&str`, so the render is passed straight through with no owned +`String`: + +```rust +let start_str: &str = if self.all_day { + start_owned = format!("{}T000000Z", start_time.format("%Y%m%d")); &start_owned +} else if let Some(s) = fmt::compact_ical_utc(&mut sbuf, start_time.timestamp()) { + s // 0 allocs, the common case +} else { + start_owned = format!("{}", start_time.format("%Y%m%dT%H%M%SZ")); &start_owned // fallback +}; +``` + +The all-day `%Y%m%d` + literal-suffix form is unchanged (no existing +no-separator helper covers it — see *Not shipped*). The 20 calendar_event unit +tests (iCal round-trip, exception handling) pass unchanged. + +## [S1] `ShareItemType::try_from` — `eq_ignore_ascii_case` + +`match s.to_lowercase().as_str()` allocated a Unicode-lowercased `String` per +call only to compare against `"file"`/`"folder"`. `eq_ignore_ascii_case` folds +only ASCII A–Z — but the targets are pure ASCII, and any input whose +`to_lowercase()` equals `"file"`/`"folder"` is by definition an ASCII case +variant of it, so acceptance is byte-identical (the gate checks mixed-case + +invalid inputs). 0 allocs. + +## Not shipped — deferred to a later round + +Surfaced by the Round-22 audit (three parallel sub-audits across the HTTP, DAV +and application/parse layers), verified against current source, but held back — +each needs a signature/API decision or a gate the deterministic alloc-counter +can't provide: + +- **`list_files_query` `Query>` → typed `Query<…>`**: the + listing reads only `folder_id`, so a `struct ListFilesQuery { folder_id: + Option }` drops the `HashMap` table + the `"folder_id"` key `String` + (~3 → 1 allocs). Byte-identical for the frontend's actual usage, but a + **malformed** `?folder_id=a&folder_id=b` diverges (HashMap last-wins vs serde + field-decode), so it wants its own byte-identity proof before shipping — the + H1 half of this handler is unimpeachable and shipped alone. +- **NextCloud `avatar` HeaderMap clone**: `handle_avatar` has two callers + (`handle_dav_avatar` + a direct route), so the `Request` conversion is a + dual-signature change, not the clean leaf swap the other H1 handlers were. + Low frequency (avatars revalidate hourly). +- **Share-management HeaderMap clones** (`create`/`update`/`verify`/… at + `share_handler.rs:583+`): these take a `Json` body (a `FromRequest` body + extractor), so a second `Request` extractor is impossible — they need a + different borrow strategy. Lower frequency than the public download/access + path shipped here. +- **`update_all_day` / `update_time_range` all-day `%Y%m%d` stamp**: no + no-separator date helper exists (`compact_ical_utc` is date+time, + `compact_date` is `YYYY-MM-DD`); a `compact_date_basic` (`YYYYMMDD`) would + close the remaining 2 all-day sites. Low heat. +- **`ContactService::generate_vcard` BDAY** (`contact_service.rs:342`) still uses + `birthday.format("%Y%m%d")` on the contact write path — same missing + `%Y%m%d` helper as above; the per-contact *read* twin was already fixed + (ROUND21 §R5). Low heat. +- **`extract_webdav_path(req.uri())`** (`webdav_handler.rs:507`): a per-PROPFIND + percent-decode + `String`, but byte-identity is **unproven** — the code + comment states the `path` parameter carries a home-folder prefix that is + wrong for WebDAV hrefs, directly contradicting ROUND21's "stale comment" + note. Needs a dedicated href-equivalence proof, not a perf banner. + +## Environment / methodology + +- `cargo run --release --features bench --example bench_round22_micro` — + counting global allocator, no Postgres. Tunable (env): `BENCH_ITERS` (200000). +- Each section is BEFORE (verbatim replica of the shipped-before shape) vs AFTER + (verbatim replica of the shipped-after shape, which the source is then made to + match), with a byte/-value equivalence gate; the shipped source now matches + each AFTER arm. +- Roll-back rule encoded per section: the harness `std::process::exit(1)`s with + `GATE FAIL … rollback` if an AFTER arm fails to reduce allocations. +- Verified beyond the bench: `cargo clippy --features bench --all-targets -D + warnings` clean, `cargo fmt --all --check` clean, and `cargo test --lib + --features bench` = **529 passed / 0 failed** (incl. the OpenAPI-spec-validity + test that guards the H1 utoipa-handler signature change). +``` diff --git a/examples/bench_round22_micro.rs b/examples/bench_round22_micro.rs new file mode 100644 index 00000000..2eba3b5b --- /dev/null +++ b/examples/bench_round22_micro.rs @@ -0,0 +1,521 @@ +//! Round-22 CPU/alloc micro-pack (no Postgres). +//! +//! Same rule as ROUND2–21: each section is BEFORE (verbatim replica of the +//! shipped-before shape) vs AFTER (verbatim replica of the shipped-after shape, +//! which the source is then made to match), with a byte/-value equivalence gate +//! and a `GATE FAIL … rollback` check that `std::process::exit(1)`s if the AFTER +//! arm fails to beat its BEFORE — the round's roll-back rule encoded into the +//! benchmark. An AFTER that doesn't win is never applied to the source. +//! +//! [H1] Hot GET handlers (`get_thumbnail`, `download_file`, `list_files`, +//! `list_photos`, NC `preview`, public-share download/access) took the +//! axum `HeaderMap` extractor, which does `parts.headers.clone()` — an +//! owned clone of the whole request header table (~2 allocs) — just to +//! read 1–3 headers. AFTER takes `req: Request` and reads `req.headers()` +//! by borrow (the ROUND14 §A4 middleware pattern applied to the handlers). +//! +//! [W1] The native WebDAV `write_etag_quoted` (per file AND per folder of +//! every `/webdav/` PROPFIND row, up to 500/page — the most-travelled +//! DAV path) built a `"{etag}"` String then wrote it auto-escaped; +//! `quick_xml` escapes the `"` → `"`, re-allocating an owned `Cow`, +//! so 2 allocs/row (buffer + escape). AFTER emits the quotes as borrowed +//! pre-escaped `"` events (the ROUND20 §C1 / ROUND21 §R4 pattern). +//! +//! [C1] The CalDAV `getetag` emit (per event of every calendar REPORT/multiget, +//! per calendar of the home-set PROPFIND) still escaped a `"…"` value +//! (reused buffer → 1 alloc/event escape; `format!` calendar sites → 2). +//! AFTER routes all five sites through a `write_quoted_etag` helper +//! (borrowed pre-escaped quotes) — 0 allocs. +//! +//! [D1] `FileDto::from` cloned `content_hash` via `file.content_hash() +//! .to_string()` and then `into_parts()` MOVED the same `blob_hash` into +//! `parts.blob_hash`, which was dropped unused — 1 wasted alloc on every +//! listing row. AFTER reuses `parts.blob_hash` (0). +//! +//! [E1] `CalendarEvent::update_time_range` / `update_all_day` stamped timed +//! DTSTART/DTEND via `format!("{}", t.format("%Y%m%dT%H%M%SZ"))` — chrono's +//! strftime interpreter (~3 allocs). AFTER stack-renders via the shipped +//! `fmt::compact_ical_utc` and passes the `&str` straight to +//! `update_ical_property` (0 allocs), chrono fallback out of range. +//! +//! [S1] `ShareItemType::try_from` matched `s.to_lowercase().as_str()` — a +//! throwaway Unicode-lowercased String — against two ASCII literals. +//! AFTER uses `eq_ignore_ascii_case` (byte-identical acceptance, 0 allocs). +//! +//! Run: +//! cargo run --release --features bench --example bench_round22_micro +//! Tunables (env): BENCH_ITERS (200000) + +use std::alloc::{GlobalAlloc, Layout, System}; +use std::env; +use std::hint::black_box; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::time::Instant; + +use axum::http::{HeaderMap, HeaderName, HeaderValue, header}; +use chrono::{TimeZone, Utc}; +use quick_xml::Writer; +use quick_xml::events::{BytesEnd, BytesStart, BytesText, Event}; + +static ALLOC_CALLS: AtomicU64 = AtomicU64::new(0); + +struct CountingAlloc; + +unsafe impl GlobalAlloc for CountingAlloc { + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + ALLOC_CALLS.fetch_add(1, Ordering::Relaxed); + unsafe { System.alloc(layout) } + } + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + unsafe { System.dealloc(ptr, layout) } + } + unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { + ALLOC_CALLS.fetch_add(1, Ordering::Relaxed); + unsafe { System.realloc(ptr, layout, new_size) } + } + unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { + ALLOC_CALLS.fetch_add(1, Ordering::Relaxed); + unsafe { System.alloc_zeroed(layout) } + } +} + +#[global_allocator] +static GLOBAL: CountingAlloc = CountingAlloc; + +fn env_or(key: &str, default: T) -> T { + env::var(key) + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(default) +} + +struct Measured { + wall_ns_per_op: f64, + allocs_per_op: f64, +} + +fn measure(iters: usize, mut f: F) -> Measured { + // Warm up (grow any reused buffers, prime caches) so the measured window + // reflects steady state, not first-touch growth. + for _ in 0..(iters / 20).max(1) { + f(); + } + let a0 = ALLOC_CALLS.load(Ordering::Relaxed); + let t = Instant::now(); + for _ in 0..iters { + f(); + } + let wall = t.elapsed().as_nanos() as f64 / iters as f64; + let allocs = (ALLOC_CALLS.load(Ordering::Relaxed) - a0) as f64 / iters as f64; + Measured { + wall_ns_per_op: wall, + allocs_per_op: allocs, + } +} + +fn print_row(label: &str, m: &Measured) { + println!( + "| {:<52} | {:>12.1} | {:>10.2} |", + label, m.wall_ns_per_op, m.allocs_per_op + ); +} + +fn header_footer(name: &str, before: &Measured, after: &Measured) { + println!("| arm | ns/op | allocs/op |"); + print_row(&format!("BEFORE {name}"), before); + print_row(&format!("AFTER {name}"), after); + println!( + "# {:.2}x wall, {:.2} fewer allocs/op", + before.wall_ns_per_op / after.wall_ns_per_op, + before.allocs_per_op - after.allocs_per_op + ); +} + +fn gate_allocs(tag: &str, before: &Measured, after: &Measured) { + if after.allocs_per_op >= before.allocs_per_op { + eprintln!("GATE FAIL [{tag}]: AFTER did not reduce allocations — rollback"); + std::process::exit(1); + } +} + +// ──────────────────────────────────────────────────────────────────────────── +// [H1] Hot GET handler HeaderMap extractor — axum `HeaderMap` (clones the whole +// request header table via `parts.headers.clone()`) vs `Request` + borrow. +// ──────────────────────────────────────────────────────────────────────────── + +/// A realistic browser GET request header set (what a thumbnail / photo-list / +/// download request actually carries). The `HeaderMap` extractor clones ALL of +/// it just so the handler can read 1–3 headers (IF_NONE_MATCH / ACCEPT / RANGE). +fn realistic_request_headers() -> HeaderMap { + let mut h = HeaderMap::new(); + h.insert(header::HOST, HeaderValue::from_static("cloud.example.com")); + h.insert( + header::USER_AGENT, + HeaderValue::from_static( + "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) \ + Chrome/126.0 Safari/537.36", + ), + ); + h.insert( + header::ACCEPT, + HeaderValue::from_static("image/avif,image/webp,image/apng,image/*,*/*;q=0.8"), + ); + h.insert( + header::ACCEPT_ENCODING, + HeaderValue::from_static("gzip, deflate, br, zstd"), + ); + h.insert( + header::ACCEPT_LANGUAGE, + HeaderValue::from_static("en-US,en;q=0.9,es;q=0.8"), + ); + h.insert( + header::REFERER, + HeaderValue::from_static("https://cloud.example.com/photos"), + ); + h.insert(header::CONNECTION, HeaderValue::from_static("keep-alive")); + h.insert( + HeaderName::from_static("sec-fetch-dest"), + HeaderValue::from_static("image"), + ); + h.insert( + HeaderName::from_static("sec-fetch-mode"), + HeaderValue::from_static("no-cors"), + ); + h.insert( + HeaderName::from_static("sec-fetch-site"), + HeaderValue::from_static("same-origin"), + ); + h.insert( + header::COOKIE, + HeaderValue::from_static( + "oxicloud_session=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.payload.signature; csrf=abc123", + ), + ); + h.insert( + header::IF_NONE_MATCH, + HeaderValue::from_static("\"thumb-6b1e9f00-preview-webp\""), + ); + h +} + +fn section_h1() { + let iters: usize = env_or("BENCH_ITERS", 200_000); + let headers = realistic_request_headers(); + + // Equivalence: both arms read the identical IF_NONE_MATCH value. + let cloned = headers.clone(); + let borrowed = headers + .get(header::IF_NONE_MATCH) + .and_then(|v| v.to_str().ok()); + assert_eq!( + cloned + .get(header::IF_NONE_MATCH) + .and_then(|v| v.to_str().ok()), + borrowed, + "H1 read value differs" + ); + + // BEFORE: axum's `HeaderMap` extractor materializes an owned clone of the + // whole request header table (`parts.headers.clone()`), then the handler + // reads one header out of it. + let before = measure(iters, || { + let owned = black_box(&headers).clone(); + black_box( + owned + .get(header::IF_NONE_MATCH) + .and_then(|v| v.to_str().ok()), + ); + }); + + // AFTER: take `req: Request` and read `req.headers()` by borrow — the header + // table is never cloned; the same value is read straight from the borrow. + let after = measure(iters, || { + let borrow: &HeaderMap = black_box(&headers); + black_box( + borrow + .get(header::IF_NONE_MATCH) + .and_then(|v| v.to_str().ok()), + ); + }); + + println!( + "\n## [H1] Hot GET handler HeaderMap clone (per thumbnail/photo/download/preview/share req)" + ); + header_footer("HeaderMap::clone() vs &HeaderMap borrow", &before, &after); + gate_allocs("H1", &before, &after); +} + +// ──────────────────────────────────────────────────────────────────────────── +// [W1] Native WebDAV getetag — sized String + escape vs borrowed pre-escaped. +// ──────────────────────────────────────────────────────────────────────────── + +/// BEFORE (verbatim `webdav_adapter::write_etag_quoted`): sized `"{etag}"` +/// String then auto-escaped write — `quick_xml` escapes the `"` → `"`, +/// re-allocating an owned `Cow`. 2 allocs/row (buffer + escape). +fn w1_before(buf: &mut Vec, etag: &str) { + let mut w = Writer::new(&mut *buf); + w.write_event(Event::Start(BytesStart::new("D:getetag"))) + .unwrap(); + let mut quoted = String::with_capacity(etag.len() + 2); + quoted.push('"'); + quoted.push_str(etag); + quoted.push('"'); + w.write_event(Event::Text(BytesText::new("ed))).unwrap(); + w.write_event(Event::End(BytesEnd::new("D:getetag"))) + .unwrap(); +} + +/// AFTER: borrowed pre-escaped `"` quotes around the escaped body. +fn w1_after(buf: &mut Vec, etag: &str) { + let mut w = Writer::new(&mut *buf); + w.write_event(Event::Start(BytesStart::new("D:getetag"))) + .unwrap(); + w.write_event(Event::Text(BytesText::from_escaped("""))) + .unwrap(); + w.write_event(Event::Text(BytesText::new(etag))).unwrap(); + w.write_event(Event::Text(BytesText::from_escaped("""))) + .unwrap(); + w.write_event(Event::End(BytesEnd::new("D:getetag"))) + .unwrap(); +} + +fn section_w1() { + let iters: usize = env_or("BENCH_ITERS", 200_000); + let etag = "d41d8cd98f00b204e9800998ecf8427e-1719792000"; // realistic file etag + + // Equivalence: byte-identical output, incl. an etag with XML-special chars. + let (mut b1, mut b2) = (Vec::new(), Vec::new()); + w1_before(&mut b1, etag); + w1_after(&mut b2, etag); + assert_eq!(b1, b2, "W1 emitted bytes differ (hex etag)"); + let (mut s1, mut s2) = (Vec::new(), Vec::new()); + w1_before(&mut s1, "a&b, etag_buf: &mut String, id: &str) { + let mut w = Writer::new(&mut *buf); + w.write_event(Event::Start(BytesStart::new("D:getetag"))) + .unwrap(); + etag_buf.clear(); + etag_buf.push('"'); + etag_buf.push_str(id); + etag_buf.push('"'); + w.write_event(Event::Text(BytesText::new(etag_buf.as_str()))) + .unwrap(); + w.write_event(Event::End(BytesEnd::new("D:getetag"))) + .unwrap(); +} + +/// AFTER (`write_quoted_etag`): borrowed pre-escaped quotes; the UUID body is a +/// borrow (no XML-special chars). 0 allocs/event. +fn c1_after(buf: &mut Vec, id: &str) { + let mut w = Writer::new(&mut *buf); + w.write_event(Event::Start(BytesStart::new("D:getetag"))) + .unwrap(); + w.write_event(Event::Text(BytesText::from_escaped("""))) + .unwrap(); + w.write_event(Event::Text(BytesText::new(id))).unwrap(); + w.write_event(Event::Text(BytesText::from_escaped("""))) + .unwrap(); + w.write_event(Event::End(BytesEnd::new("D:getetag"))) + .unwrap(); +} + +fn section_c1() { + let iters: usize = env_or("BENCH_ITERS", 200_000); + let id = "6b1e9f00-4c2a-4f1e-9b7a-2d5e8c1f0a3b"; // calendar / event UUID + + // Equivalence: byte-identical output for the UUID body. + let (mut b1, mut eb, mut b2) = (Vec::new(), String::new(), Vec::new()); + c1_before(&mut b1, &mut eb, id); + c1_after(&mut b2, id); + assert_eq!(b1, b2, "C1 emitted bytes differ"); + + let mut buf = Vec::with_capacity(96); + let mut etag_buf = String::with_capacity(40); + let before = measure(iters, || { + buf.clear(); + c1_before(black_box(&mut buf), black_box(&mut etag_buf), black_box(id)); + }); + let after = measure(iters, || { + buf.clear(); + c1_after(black_box(&mut buf), black_box(id)); + }); + + println!("\n## [C1] CalDAV getetag (per event of every REPORT/multiget, per calendar)"); + header_footer("reused-buffer escape vs borrowed events", &before, &after); + gate_allocs("C1", &before, &after); +} + +// ──────────────────────────────────────────────────────────────────────────── +// [D1] FileDto::from content_hash — getter clone (moved parts.blob_hash dropped) +// vs reuse the moved String. +// ──────────────────────────────────────────────────────────────────────────── + +/// The 64-char BLAKE3-hex String a `File` owns in `blob_hash` (allocated in both +/// arms — the baseline; the delta is exactly the `content_hash` clone). +fn make_hash() -> String { + "d41d8cd98f00b204e9800998ecf8427ed41d8cd98f00b204e9800998ecf8427e".to_string() +} + +/// BEFORE: `content_hash = file.content_hash().to_string()` clones the hash, +/// then `into_parts()` moves the *same* `blob_hash` into `parts.blob_hash`, +/// which is dropped unused in the `Self { … }` ctor. +fn d1_before(owned_hash: String) -> String { + let content_hash = owned_hash.clone(); // File::content_hash().to_string() + let parts_blob_hash = owned_hash; // into_parts() moves blob_hash + let _ = parts_blob_hash; // dropped unused in the Self{} ctor + content_hash +} + +/// AFTER: `content_hash: parts.blob_hash` — reuse the moved String, no clone. +fn d1_after(owned_hash: String) -> String { + owned_hash +} + +fn section_d1() { + let iters: usize = env_or("BENCH_ITERS", 200_000); + + // Equivalence: identical content_hash string. + assert_eq!( + d1_before(make_hash()), + d1_after(make_hash()), + "D1 content_hash differs" + ); + + let before = measure(iters, || { + black_box(d1_before(black_box(make_hash()))); + }); + let after = measure(iters, || { + black_box(d1_after(black_box(make_hash()))); + }); + + println!("\n## [D1] FileDto::from content_hash (per file row of every listing)"); + header_footer("getter clone + drop moved vs reuse moved", &before, &after); + gate_allocs("D1", &before, &after); +} + +// ──────────────────────────────────────────────────────────────────────────── +// [E1] CalendarEvent timed DTSTART/DTEND — chrono strftime vs compact_ical_utc. +// ──────────────────────────────────────────────────────────────────────────── + +/// BEFORE: `format!("{}", t.format("%Y%m%dT%H%M%SZ"))` — chrono's strftime +/// interpreter builds a `DelayedFormat` and formats six fields through +/// `core::fmt`, heap-allocating. +fn e1_before(dt: chrono::DateTime) -> String { + format!("{}", dt.format("%Y%m%dT%H%M%SZ")) +} + +fn section_e1() { + let iters: usize = env_or("BENCH_ITERS", 200_000); + let secs: i64 = 1_752_753_434; // 2025-07-17T11:57:14Z + let dt = Utc.timestamp_opt(secs, 0).unwrap(); + + // Equivalence: the stack render equals the chrono strftime output. + let mut ebuf = [0u8; 16]; + let after_str = oxicloud::common::fmt::compact_ical_utc(&mut ebuf, secs).expect("in range"); + assert_eq!(e1_before(dt), after_str, "E1 stamp differs"); + + let before = measure(iters, || { + black_box(e1_before(black_box(dt))); + }); + // AFTER: stack render via the shipped helper; the `&str` is passed straight + // to `update_ical_property` in the source — 0 allocs. + let after = measure(iters, || { + let mut buf = [0u8; 16]; + black_box(oxicloud::common::fmt::compact_ical_utc( + &mut buf, + black_box(secs), + )); + }); + + println!("\n## [E1] CalendarEvent timed DTSTART/DTEND (per event-edit PUT)"); + header_footer("chrono %Y%m%dT%H%M%SZ vs compact_ical_utc", &before, &after); + gate_allocs("E1", &before, &after); +} + +// ──────────────────────────────────────────────────────────────────────────── +// [S1] ShareItemType::try_from — to_lowercase() String vs eq_ignore_ascii_case. +// ──────────────────────────────────────────────────────────────────────────── + +/// BEFORE: `s.to_lowercase().as_str()` — a throwaway Unicode-lowercased String +/// (always allocates) — matched against two ASCII literals. +fn s1_before(s: &str) -> u8 { + match s.to_lowercase().as_str() { + "file" => 0, + "folder" => 1, + _ => 2, + } +} + +/// AFTER: `eq_ignore_ascii_case` — allocation-free, byte-identical acceptance +/// for the ASCII targets. +fn s1_after(s: &str) -> u8 { + if s.eq_ignore_ascii_case("file") { + 0 + } else if s.eq_ignore_ascii_case("folder") { + 1 + } else { + 2 + } +} + +fn section_s1() { + let iters: usize = env_or("BENCH_ITERS", 200_000); + + // Equivalence across mixed case + invalid input. + for s in ["file", "File", "FOLDER", "folder", "Folder", "bogus", ""] { + assert_eq!(s1_before(s), s1_after(s), "S1 verdict differs for {s:?}"); + } + + let sample = "Folder"; // mixed-case → to_lowercase allocates + let before = measure(iters, || { + black_box(s1_before(black_box(sample))); + }); + let after = measure(iters, || { + black_box(s1_after(black_box(sample))); + }); + + println!("\n## [S1] ShareItemType::try_from (per share item-type parse)"); + header_footer( + "to_lowercase() String vs eq_ignore_ascii_case", + &before, + &after, + ); + gate_allocs("S1", &before, &after); +} + +fn main() { + println!("# Round-22 micro-pack — BEFORE/AFTER (counting allocator, release)"); + println!("# allocs/op is the deterministic gate; a non-winning AFTER exits 1 (rollback)."); + section_h1(); + section_w1(); + section_c1(); + section_d1(); + section_e1(); + section_s1(); + println!("\nAll Round-22 sections passed their allocation gate."); +} diff --git a/src/application/adapters/caldav_adapter.rs b/src/application/adapters/caldav_adapter.rs index 421902e1..c01afef6 100644 --- a/src/application/adapters/caldav_adapter.rs +++ b/src/application/adapters/caldav_adapter.rs @@ -17,6 +17,23 @@ use crate::application::adapters::webdav_adapter::{ }; use crate::application::dtos::calendar_dto::{CalendarDto, CalendarEventDto}; +/// Emit a WebDAV `getetag` body as `"…"` with the surrounding quotes written as +/// borrowed pre-escaped `"` text events around the escaped etag body. +/// +/// Byte-identical to escaping a `"{etag}"` String — `quick_xml`'s +/// `BytesText::new` escapes a literal `"` → `"`, re-allocating an owned +/// `Cow` — but with 0 heap allocs (the NextCloud ROUND20 §C1 / CardDAV +/// ROUND21 §R4 pattern, applied to the CalDAV emitter it missed). The caller +/// writes the surrounding `…` tags. Every `etag` body +/// here is a bare `Uuid` (`calendar.id` / `anchor.id`), so the escaped body is +/// itself a borrow — 0 allocs/row. +fn write_quoted_etag(xml_writer: &mut Writer, etag: &str) -> Result<()> { + xml_writer.write_event(Event::Text(BytesText::from_escaped(""")))?; + xml_writer.write_event(Event::Text(BytesText::new(etag)))?; + xml_writer.write_event(Event::Text(BytesText::from_escaped(""")))?; + Ok(()) +} + /// Parse a CalDAV `time-range` element's `start` / `end` attribute /// value into a UTC `DateTime`. /// @@ -794,9 +811,9 @@ impl CalDavAdapter { Self::write_lastmodified_text(xml_writer, calendar.updated_at)?; xml_writer.write_event(Event::End(BytesEnd::new("D:getlastmodified")))?; - // ETag + // ETag (borrowed pre-escaped quotes, §C1/§R4 — was format! + escape) xml_writer.write_event(Event::Start(BytesStart::new("D:getetag")))?; - xml_writer.write_event(Event::Text(BytesText::new(&format!("\"{}\"", calendar.id))))?; + write_quoted_etag(xml_writer, &calendar.id)?; xml_writer.write_event(Event::End(BytesEnd::new("D:getetag")))?; // Content type for calendar collection @@ -924,10 +941,7 @@ impl CalDavAdapter { } ("DAV:", "getetag") => { xml_writer.write_event(Event::Start(BytesStart::new("D:getetag")))?; - xml_writer.write_event(Event::Text(BytesText::new(&format!( - "\"{}\"", - calendar.id - ))))?; + write_quoted_etag(xml_writer, &calendar.id)?; xml_writer.write_event(Event::End(BytesEnd::new("D:getetag")))?; } ("DAV:", "getcontenttype") => { @@ -1117,11 +1131,11 @@ impl CalDavAdapter { events: &[CalendarEventDto], base_href: &str, ) -> Result<()> { - // Reused per-event buffers (cleared each iteration) so a whole PROPFIND - // page allocates the href/etag storage once instead of twice per event - // (benches/ROUND14.md §A6). + // Reused per-event href buffer (cleared each iteration) so a whole + // PROPFIND page allocates the href storage once instead of per event + // (benches/ROUND14.md §A6). The etag no longer needs a buffer — it is + // emitted via `write_quoted_etag` (borrowed pre-escaped quotes). let mut event_href = String::with_capacity(base_href.len() + 48); - let mut etag = String::new(); for bundle in group_events_by_uid(events) { // The master (sorted first by group_events_by_uid) // supplies the ETag anchor + getlastmodified. If @@ -1148,11 +1162,9 @@ impl CalDavAdapter { // resourcetype (empty for non-collection) xml_writer.write_event(Event::Empty(BytesStart::new("D:resourcetype")))?; - // getetag — anchor row's id (reused buffer, benches/ROUND14.md §A6) + // getetag — anchor row's id (borrowed pre-escaped quotes, §C1/§R4) xml_writer.write_event(Event::Start(BytesStart::new("D:getetag")))?; - etag.clear(); - let _ = std::fmt::Write::write_fmt(&mut etag, format_args!("\"{}\"", anchor.id)); - xml_writer.write_event(Event::Text(BytesText::new(&etag)))?; + write_quoted_etag(xml_writer, &anchor.id)?; xml_writer.write_event(Event::End(BytesEnd::new("D:getetag")))?; // getcontenttype @@ -1215,10 +1227,10 @@ impl CalDavAdapter { CalDavReportType::CalendarMultiget { props, .. } => props, CalDavReportType::SyncCollection { props, .. } => props, }; - // Reused per-event href + etag buffers for the whole REPORT page - // (benches/ROUND14.md §A6). + // Reused per-event href buffer for the whole REPORT page + // (benches/ROUND14.md §A6). The etag is emitted via `write_quoted_etag` + // (borrowed pre-escaped quotes) and no longer needs a buffer. let mut href = String::with_capacity(base_href.len() + 48); - let mut etag = String::new(); for bundle in group_events_by_uid(events) { let anchor = match bundle.first() { Some(e) => *e, @@ -1229,7 +1241,7 @@ impl CalDavAdapter { &mut href, format_args!("{}{}.ics", base_href, anchor.ical_uid), ); - Self::write_event_response(xml_writer, &bundle, props, &href, &mut etag)?; + Self::write_event_response(xml_writer, &bundle, props, &href)?; } Ok(()) } @@ -1266,7 +1278,6 @@ impl CalDavAdapter { bundle: &[&CalendarEventDto], props: &[QualifiedName], href: &str, - etag: &mut String, ) -> Result<()> { let anchor = bundle .first() @@ -1289,10 +1300,10 @@ impl CalDavAdapter { // If no specific props requested, return all common ones if props.is_empty() { - Self::write_event_standard_props(xml_writer, anchor, bundle, etag)?; + Self::write_event_standard_props(xml_writer, anchor, bundle)?; } else { // Write specifically requested properties - Self::write_event_requested_props(xml_writer, anchor, bundle, props, etag)?; + Self::write_event_requested_props(xml_writer, anchor, bundle, props)?; } // End prop @@ -1320,22 +1331,17 @@ impl CalDavAdapter { xml_writer: &mut Writer, anchor: &CalendarEventDto, bundle: &[&CalendarEventDto], - etag: &mut String, ) -> Result<()> { // Common WebDAV properties // Resource type (empty for non-collection) xml_writer.write_event(Event::Empty(BytesStart::new("D:resourcetype")))?; - // ETag anchored on the master (or first exception in - // a master-less bundle — pathological state today). - // Reused buffer (benches/ROUND14.md §A6). + // ETag anchored on the master (or first exception in a master-less + // bundle — pathological state today). Borrowed pre-escaped quotes + // (§C1/§R4), 0 allocs/event. xml_writer.write_event(Event::Start(BytesStart::new("D:getetag")))?; - etag.clear(); - etag.push('"'); - etag.push_str(&anchor.id); - etag.push('"'); - xml_writer.write_event(Event::Text(BytesText::new(etag.as_str())))?; + write_quoted_etag(xml_writer, &anchor.id)?; xml_writer.write_event(Event::End(BytesEnd::new("D:getetag")))?; // Content type @@ -1370,7 +1376,6 @@ impl CalDavAdapter { anchor: &CalendarEventDto, bundle: &[&CalendarEventDto], props: &[QualifiedName], - etag: &mut String, ) -> Result<()> { for prop in props { match (prop.namespace.as_str(), prop.name.as_str()) { @@ -1380,12 +1385,7 @@ impl CalDavAdapter { } ("DAV:", "getetag") => { xml_writer.write_event(Event::Start(BytesStart::new("D:getetag")))?; - // Reused buffer (benches/ROUND14.md §A6). - etag.clear(); - etag.push('"'); - etag.push_str(&anchor.id); - etag.push('"'); - xml_writer.write_event(Event::Text(BytesText::new(etag.as_str())))?; + write_quoted_etag(xml_writer, &anchor.id)?; xml_writer.write_event(Event::End(BytesEnd::new("D:getetag")))?; } ("DAV:", "getcontenttype") => { diff --git a/src/application/adapters/webdav_adapter.rs b/src/application/adapters/webdav_adapter.rs index 15623f3b..b6f31c84 100644 --- a/src/application/adapters/webdav_adapter.rs +++ b/src/application/adapters/webdav_adapter.rs @@ -809,12 +809,17 @@ impl WebDavAdapter { fn write_etag_quoted(xml_writer: &mut Writer, etag: &str) -> Result<()> { xml_writer.write_event(Event::Start(BytesStart::new("D:getetag")))?; - // One exactly-sized allocation instead of format!'s grow-from-empty. - let mut quoted = String::with_capacity(etag.len() + 2); - quoted.push('"'); - quoted.push_str(etag); - quoted.push('"'); - xml_writer.write_event(Event::Text(BytesText::new("ed)))?; + // Borrowed pre-escaped quotes (the ROUND20 §C1 NextCloud / ROUND21 §R4 + // CardDAV pattern the native WebDAV adapter never got): `BytesText::new` + // escapes a literal `"` → `"`, re-allocating an owned `Cow`, so the + // old `"{etag}"` String paid TWO allocs/row (the sized buffer + the + // escape). Emit the two quotes as borrowed pre-escaped `"` text + // events around the escaped body — byte-identical output, 0 allocs/row + // on the hottest native-WebDAV PROPFIND path (per file AND per folder, + // up to PROPFIND_BATCH_SIZE=500 rows/page). + xml_writer.write_event(Event::Text(BytesText::from_escaped(""")))?; + xml_writer.write_event(Event::Text(BytesText::new(etag)))?; + xml_writer.write_event(Event::Text(BytesText::from_escaped(""")))?; xml_writer.write_event(Event::End(BytesEnd::new("D:getetag")))?; Ok(()) } diff --git a/src/application/dtos/file_dto.rs b/src/application/dtos/file_dto.rs index d9074451..ba668c94 100644 --- a/src/application/dtos/file_dto.rs +++ b/src/application/dtos/file_dto.rs @@ -93,10 +93,15 @@ impl From for FileDto { // already-extracted parts. `content_hash` is just the raw // blob hash; `etag` is the cache token derived from it. let etag = file.etag(); - let content_hash = file.content_hash().to_string(); // Consume the entity by moving all fields — zero heap allocations - // for id, name, path, folder_id (previously 4× .to_string()). + // for id, name, path, folder_id (previously 4× .to_string()), and now + // for `content_hash` too: `into_parts()` moves `blob_hash` out, so it is + // reused verbatim below instead of cloning it through the + // `content_hash()` getter. The moved `parts.blob_hash` was previously + // dropped unused while the getter clone paid 1 alloc/row on every file + // listing (folder browse, streaming PROPFIND, search/favorites/recent + // hydration). `etag` is still computed first from the live entity. let parts = file.into_parts(); // Display fields come from closed static tables and MIME values @@ -123,7 +128,7 @@ impl From for FileDto { category, size_formatted, sort_date: None, - content_hash, + content_hash: parts.blob_hash, etag, created_by: parts.created_by, updated_by: parts.updated_by, diff --git a/src/domain/entities/calendar_event.rs b/src/domain/entities/calendar_event.rs index 652f3e57..2fb6c4e9 100644 --- a/src/domain/entities/calendar_event.rs +++ b/src/domain/entities/calendar_event.rs @@ -575,21 +575,38 @@ impl CalendarEvent { self.end_time = end_time; self.updated_at = Utc::now(); - // Update iCalendar data - let start_str = if self.all_day { - format!("{}T000000Z", start_time.format("%Y%m%d")) + // Update iCalendar data. Timed events stack-render the compact UTC + // stamp via `fmt::compact_ical_utc` (the ROUND19 §V2 pattern: drops + // chrono's `%Y%m%dT%H%M%SZ` strftime interpreter — ~3 → 0 allocs each), + // with chrono kept as the out-of-range fallback. All-day keeps its + // `%Y%m%d` + literal-suffix form. Byte-identical output either way. + let (mut sbuf, mut ebuf) = ([0u8; 16], [0u8; 16]); + let (start_owned, end_owned); + let start_str: &str = if self.all_day { + start_owned = format!("{}T000000Z", start_time.format("%Y%m%d")); + &start_owned + } else if let Some(s) = + crate::common::fmt::compact_ical_utc(&mut sbuf, start_time.timestamp()) + { + s } else { - format!("{}", start_time.format("%Y%m%dT%H%M%SZ")) + start_owned = format!("{}", start_time.format("%Y%m%dT%H%M%SZ")); + &start_owned + }; + let end_str: &str = if self.all_day { + end_owned = format!("{}T000000Z", end_time.format("%Y%m%d")); + &end_owned + } else if let Some(e) = + crate::common::fmt::compact_ical_utc(&mut ebuf, end_time.timestamp()) + { + e + } else { + end_owned = format!("{}", end_time.format("%Y%m%dT%H%M%SZ")); + &end_owned }; - let end_str = if self.all_day { - format!("{}T000000Z", end_time.format("%Y%m%d")) - } else { - format!("{}", end_time.format("%Y%m%dT%H%M%SZ")) - }; - - self.update_ical_property("DTSTART", &start_str); - self.update_ical_property("DTEND", &end_str); + self.update_ical_property("DTSTART", start_str); + self.update_ical_property("DTEND", end_str); Ok(()) } @@ -603,21 +620,37 @@ impl CalendarEvent { self.all_day = all_day; self.updated_at = Utc::now(); - // Update iCalendar data - let start_str = if all_day { - format!("VALUE=DATE:{}", self.start_time.format("%Y%m%d")) + // Update iCalendar data. Timed events stack-render the compact UTC + // stamp via `fmt::compact_ical_utc` (drops chrono's `%Y%m%dT%H%M%SZ` + // strftime interpreter — ~3 → 0 allocs each), chrono fallback out of + // range. All-day keeps its `VALUE=DATE:` + `%Y%m%d` form. Byte-identical. + let (mut sbuf, mut ebuf) = ([0u8; 16], [0u8; 16]); + let (start_owned, end_owned); + let start_str: &str = if all_day { + start_owned = format!("VALUE=DATE:{}", self.start_time.format("%Y%m%d")); + &start_owned + } else if let Some(s) = + crate::common::fmt::compact_ical_utc(&mut sbuf, self.start_time.timestamp()) + { + s } else { - format!("{}", self.start_time.format("%Y%m%dT%H%M%SZ")) + start_owned = format!("{}", self.start_time.format("%Y%m%dT%H%M%SZ")); + &start_owned + }; + let end_str: &str = if all_day { + end_owned = format!("VALUE=DATE:{}", self.end_time.format("%Y%m%d")); + &end_owned + } else if let Some(e) = + crate::common::fmt::compact_ical_utc(&mut ebuf, self.end_time.timestamp()) + { + e + } else { + end_owned = format!("{}", self.end_time.format("%Y%m%dT%H%M%SZ")); + &end_owned }; - let end_str = if all_day { - format!("VALUE=DATE:{}", self.end_time.format("%Y%m%d")) - } else { - format!("{}", self.end_time.format("%Y%m%dT%H%M%SZ")) - }; - - self.update_ical_property("DTSTART", &start_str); - self.update_ical_property("DTEND", &end_str); + self.update_ical_property("DTSTART", start_str); + self.update_ical_property("DTEND", end_str); } /** diff --git a/src/domain/entities/share.rs b/src/domain/entities/share.rs index 93d965e4..a77a94e2 100644 --- a/src/domain/entities/share.rs +++ b/src/domain/entities/share.rs @@ -180,13 +180,18 @@ impl TryFrom<&str> for ShareItemType { type Error = ShareError; fn try_from(s: &str) -> Result { - match s.to_lowercase().as_str() { - "file" => Ok(ShareItemType::File), - "folder" => Ok(ShareItemType::Folder), - _ => Err(ShareError::ValidationError(format!( + // ASCII case-insensitive compare against the two literals instead of a + // throwaway Unicode `to_lowercase()` String — byte-identical acceptance + // for the ASCII targets "file"/"folder" (1 → 0 allocs/call). + if s.eq_ignore_ascii_case("file") { + Ok(ShareItemType::File) + } else if s.eq_ignore_ascii_case("folder") { + Ok(ShareItemType::Folder) + } else { + Err(ShareError::ValidationError(format!( "Invalid item type: {}", s - ))), + ))) } } } diff --git a/src/interfaces/api/handlers/file_handler.rs b/src/interfaces/api/handlers/file_handler.rs index 14c6f455..c1ec1fa8 100644 --- a/src/interfaces/api/handlers/file_handler.rs +++ b/src/interfaces/api/handlers/file_handler.rs @@ -361,9 +361,9 @@ impl FileHandler { pub(super) async fn get_thumbnail_impl( State(state): State, auth_user: AuthUser, - headers: HeaderMap, + headers: &HeaderMap, Path((id, size)): Path<(String, String)>, - ) -> impl IntoResponse { + ) -> impl IntoResponse + use<> { use crate::application::ports::thumbnail_ports::{ThumbnailFormat, ThumbnailSize}; // check first that user can access this resource @@ -665,8 +665,8 @@ impl FileHandler { auth_user: AuthUser, Path(id): Path, Query(params): Query>, - headers: HeaderMap, - ) -> impl IntoResponse { + headers: &HeaderMap, + ) -> impl IntoResponse + use<> { let retrieval = &state.applications.file_retrieval_service; // ── Get file metadata (ownership-scoped) ──────────────────────── @@ -705,7 +705,7 @@ impl FileHandler { let etag = format!("\"{}\"", file_dto.etag); // ── ETag (304 Not Modified) ────────────────────────────────── - if let Some(resp) = not_modified_response(&headers, &etag) { + if let Some(resp) = not_modified_response(headers, &etag) { return resp.into_response(); } @@ -830,9 +830,9 @@ impl FileHandler { pub(super) async fn list_files_query_impl( State(state): State, auth_user: AuthUser, - headers: HeaderMap, + headers: &HeaderMap, Query(params): Query>, - ) -> impl IntoResponse { + ) -> impl IntoResponse + use<> { let folder_id = params.get("folder_id").map(|id| id.as_str()); tracing::info!("API: Listing files with folder_id: {:?}", folder_id); @@ -1217,10 +1217,14 @@ pub(super) fn build_content_disposition(name: &str, mime: &str, force_inline: bo pub async fn list_files_query( state: State, auth_user: AuthUser, - headers: HeaderMap, query: Query>, + req: axum::extract::Request, ) -> impl IntoResponse { - FileHandler::list_files_query_impl(state, auth_user, headers, query).await + // Read headers by borrow (`req.headers()`) instead of the `HeaderMap` + // extractor, which clones the whole request header table (~2 allocs) just to + // read one If-None-Match — the ROUND14 §A4 middleware pattern applied to the + // hot listing handler (benches/ROUND22.md §H1). + FileHandler::list_files_query_impl(state, auth_user, req.headers(), query).await } #[utoipa::path( @@ -1299,9 +1303,12 @@ pub async fn download_file( auth_user: AuthUser, path: Path, query: Query>, - headers: HeaderMap, + req: axum::extract::Request, ) -> impl IntoResponse { - FileHandler::download_file_impl(state, auth_user, path, query, headers).await + // Borrow the headers (`req.headers()`) instead of the `HeaderMap` extractor's + // full clone — every download AND every media Range seek hit this path + // (benches/ROUND22.md §H1). + FileHandler::download_file_impl(state, auth_user, path, query, req.headers()).await } #[utoipa::path( @@ -1323,10 +1330,13 @@ pub async fn download_file( pub async fn get_thumbnail( state: State, auth_user: AuthUser, - headers: HeaderMap, path: Path<(String, String)>, + req: axum::extract::Request, ) -> impl IntoResponse { - FileHandler::get_thumbnail_impl(state, auth_user, headers, path).await + // Borrow the headers (`req.headers()`) instead of the `HeaderMap` extractor's + // full clone — thumbnails are the highest-frequency GET (one per grid tile), + // and this handler reads only Accept + If-None-Match (benches/ROUND22.md §H1). + FileHandler::get_thumbnail_impl(state, auth_user, req.headers(), path).await } #[utoipa::path( diff --git a/src/interfaces/api/handlers/photos_handler.rs b/src/interfaces/api/handlers/photos_handler.rs index dd436054..897bdbd8 100644 --- a/src/interfaces/api/handlers/photos_handler.rs +++ b/src/interfaces/api/handlers/photos_handler.rs @@ -2,7 +2,7 @@ use axum::{ Json, body::Body, extract::{Query, State}, - http::{HeaderMap, Response, StatusCode, header}, + http::{Response, StatusCode, header}, response::IntoResponse, }; use serde::{Deserialize, Serialize}; @@ -60,9 +60,12 @@ struct PhotoDto { pub async fn list_photos( State(state): State>, auth_user: AuthUser, - headers: HeaderMap, Query(params): Query, + req: axum::extract::Request, ) -> impl IntoResponse { + // Borrow headers (`req.headers()`) instead of cloning the whole request + // header table via the `HeaderMap` extractor to read one If-None-Match — the + // gallery open + every pagination page hit this (benches/ROUND22.md §H1). let caller_id = auth_user.id; let limit = params.limit.unwrap_or(200).clamp(1, 500); @@ -88,7 +91,7 @@ pub async fn list_photos( std::hash::Hash::hash(&count, &mut hasher); let etag = format!("\"{:x}\"", std::hash::Hasher::finish(&hasher)); - if let Some(inm) = headers.get(header::IF_NONE_MATCH) + if let Some(inm) = req.headers().get(header::IF_NONE_MATCH) && let Ok(client_etag) = inm.to_str() && client_etag == etag { diff --git a/src/interfaces/api/handlers/share_handler.rs b/src/interfaces/api/handlers/share_handler.rs index ccc2c324..4343f0ad 100644 --- a/src/interfaces/api/handlers/share_handler.rs +++ b/src/interfaces/api/handlers/share_handler.rs @@ -230,10 +230,12 @@ pub async fn delete_shared_link( pub async fn access_shared_item( State(share_use_case): State>, Path(token): Path, - headers: HeaderMap, + req: axum::extract::Request, ) -> impl IntoResponse { // Honour an unlock cookie if one was issued by a prior `/verify` call. - let unlock_jwt = unlock_jwt_from_headers(&headers, &token); + // Borrow the headers (`req.headers()`) instead of the `HeaderMap` extractor's + // full clone to read the unlock cookie (benches/ROUND22.md §H1). + let unlock_jwt = unlock_jwt_from_headers(req.headers(), &token); // The access-count increment doesn't gate the fetch — run both // round-trips concurrently instead of serially (one RTT saved on @@ -333,8 +335,11 @@ pub async fn verify_shared_item_password( pub async fn download_shared_file( State(state): State>, Path(token): Path, - headers: HeaderMap, + req: axum::extract::Request, ) -> impl IntoResponse { + // Borrow the headers (`req.headers()`) instead of the `HeaderMap` extractor's + // full clone — the public-share download + Range path (benches/ROUND22.md §H1). + let headers = req.headers(); // 1. Resolve share service let share_service = match &state.share_service { Some(s) => s.clone(), @@ -349,7 +354,7 @@ pub async fn download_shared_file( }; // 2. Validate the share token (handles expiry + password checks) - let unlock_jwt = unlock_jwt_from_headers(&headers, &token); + let unlock_jwt = unlock_jwt_from_headers(headers, &token); let share_dto = match share_service .get_shared_link_with_unlock(&token, unlock_jwt.as_deref()) .await @@ -385,7 +390,7 @@ pub async fn download_shared_file( &state, &share_dto.item_id, share_dto.item_name.as_deref(), - &headers, + headers, ) .await } diff --git a/src/interfaces/nextcloud/preview_handler.rs b/src/interfaces/nextcloud/preview_handler.rs index f4fe8c4b..c137112b 100644 --- a/src/interfaces/nextcloud/preview_handler.rs +++ b/src/interfaces/nextcloud/preview_handler.rs @@ -5,7 +5,7 @@ use axum::{ body::Body, extract::{Query, State}, - http::{HeaderMap, StatusCode, header}, + http::{StatusCode, header}, response::{IntoResponse, Response}, }; use serde::Deserialize; @@ -39,7 +39,7 @@ pub async fn handle_preview( State(state): State>, user: AuthUser, Query(params): Query, - headers: HeaderMap, + req: axum::extract::Request, ) -> impl IntoResponse { // Parse the Nextcloud file ID — the NC app may append an instance suffix // (e.g. "00000326ocnca"), so strip non-digit characters first. @@ -155,7 +155,7 @@ pub async fn handle_preview( e.push('"'); e }; - if let Some(inm) = headers.get(header::IF_NONE_MATCH) + if let Some(inm) = req.headers().get(header::IF_NONE_MATCH) && let Ok(client_etag) = inm.to_str() && (client_etag == etag || client_etag == "*") { From 1ec7030cc7020892ef0b57e599f2086499e6e208 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 15:25:42 +0000 Subject: [PATCH 2/3] =?UTF-8?q?perf:=20round=2023=20=E2=80=94=20Postgres?= =?UTF-8?q?=20query-shape=20pass:=20typed=20JSONB=20decode,=20drive-policy?= =?UTF-8?q?=20borrow-deserialize,=20user-profile=20join!,=20subject-group?= =?UTF-8?q?=20CTE=20reuse,=20dedup=20unzip?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Benchmark-gated, same rule as ROUND2-22: BEFORE/AFTER with a value-equivalence gate and rollback-on-regression. Two harnesses — bench_round23_micro (no Postgres; deterministic allocation gate) and bench_round23_queries (live Postgres; p50 latency + strict equivalence gate against seeded fixtures). See benches/ROUND23.md. - J1: contact_pg_repository::row_to_contact (+ the inlined contact_group sibling) decode the 3 JSONB columns via sqlx::types::Json (one from_slice pass) instead of row.get:: + from_value (a throwaway Value DOM per column, walked a second time). Per contact row of every list / multiget / CardDAV sync. Micro 84 -> 33 allocs/op (2.15x); PG 3794 -> 2360 ns/contact (1.61x) on 500 real rows. - J2: DrivePolicies::from_value deserializes straight from the borrow (T::deserialize(&Value)) instead of from_value(value.clone()) — dropping the full-DOM clone on every drive-policy read (move/copy, share, grant); one-line body change, all 7 callers unchanged. Micro 5 -> 0 allocs/op (11.51x). - P1: get_user_profile overlaps the two independent caller+target reads with tokio::join! (self-case still a single fetch; caller-error precedence preserved via caller_res? first) instead of two serial round-trips. PG 577 -> 312 us/call (1.85x). - G1: subject_group remove_member computes the child's transitive-user recursive CTE once and reuses it for both the would-empty pre-check and the cache invalidation, instead of running the identical CTE twice (the edge delete is above the child, so its descendants can't change). PG 829 -> 412 us/removal (2.01x). - U1: dedup_service (store_loose_chunks final registration + the ingest run_rollback) reshapes the owned, dead-after Vec<(String,i64)> via into_iter().unzip() instead of cloning every 64-byte hash for the sync_blobs(&[String]) + UNNEST bind. Micro 256 -> 0 hash clones. Verified: cargo clippy --features bench --all-targets -D warnings clean, cargo fmt --all --check clean, cargo test --lib --features bench = 529 passed / 0 failed. The PG benches run against a local PostgreSQL 16 (schema applied from migrations/); every equivalence gate passes. The download_zip per-item N+1 (the audit's highest raw-latency candidate) is deferred to a dedicated pass: its fix moves the sole authorization inside the stream call, so it needs an AuthZ-ordering + anti-enumeration proof, not a perf banner. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01DKyQ4AnYtgp1JtjzweyMeo --- Cargo.toml | 21 + benches/ROUND23.md | 193 ++++++++ examples/bench_round23_micro.rs | 363 +++++++++++++++ examples/bench_round23_queries.rs | 433 ++++++++++++++++++ .../services/auth_application_service.rs | 19 +- .../services/subject_group_service.rs | 44 +- src/domain/entities/drive.rs | 10 +- .../pg/contact_group_pg_repository.rs | 24 +- .../repositories/pg/contact_pg_repository.rs | 29 +- src/infrastructure/services/dedup_service.rs | 13 +- 10 files changed, 1107 insertions(+), 42 deletions(-) create mode 100644 benches/ROUND23.md create mode 100644 examples/bench_round23_micro.rs create mode 100644 examples/bench_round23_queries.rs diff --git a/Cargo.toml b/Cargo.toml index a678a0c0..71ee207f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -354,6 +354,27 @@ name = "bench_micro_allocs" path = "examples/bench_micro_allocs.rs" required-features = ["bench"] +# Round-23 battery ──────────────────────────────────────────────────────────── + +# Round-23 CPU/alloc micro-pack (no Postgres) — deterministic alloc gates for +# the decode/clone candidates: contact JSONB `Value`+`from_value` → typed +# `sqlx::types::Json` decode (J1); `DrivePolicies::from_value` DOM clone → +# borrow-deserialize (J2); dedup hash reshape clone-collect → `into_iter().unzip()` +# (U1). The PG latency/round-trip/equivalence evidence is bench_round23_queries. +[[example]] +name = "bench_round23_micro" +path = "examples/bench_round23_micro.rs" +required-features = ["bench"] + +# Round-23 PG query-shape pack — end-to-end latency + equivalence on the live +# dev Postgres: contact JSONB typed decode on real rows (Q1); get_user_profile +# 2 serial reads → tokio::join! (Q4); subject_group remove_member 2 recursive +# CTEs → 1 reused (Q6). Needs the dev Postgres up (reads DATABASE_URL from .env). +[[example]] +name = "bench_round23_queries" +path = "examples/bench_round23_queries.rs" +required-features = ["bench"] + # Round-22 battery ──────────────────────────────────────────────────────────── # Round-22 CPU/alloc micro-pack — the deferred hot-GET-handler `HeaderMap` diff --git a/benches/ROUND23.md b/benches/ROUND23.md new file mode 100644 index 00000000..d3a18f0c --- /dev/null +++ b/benches/ROUND23.md @@ -0,0 +1,193 @@ +# Round 23 — Postgres query-shape pass: typed JSONB decode, drive-policy borrow-deserialize, user-profile join!, subject-group CTE reuse, dedup unzip + +Benchmark-gated, same rule as ROUND2–22: every change ships with a BEFORE/AFTER +benchmark and a value equivalence gate; an AFTER that doesn't beat its BEFORE is +rolled back (never applied). This round is the **PostgreSQL** pass — the +candidates the earlier rounds deferred as "needs a database to bench" — so it +ships two harnesses: + +- **`bench_round23_micro`** (no Postgres) — the deterministic **allocation gate** + for the decode/clone candidates (counting global allocator; a non-winning + AFTER `std::process::exit(1)`s with `GATE FAIL … rollback`). +- **`bench_round23_queries`** (live Postgres) — end-to-end **p50 latency** + a + strict **equivalence gate** (identical decoded rows / ids / user-sets from + BEFORE and AFTER; a mismatch exits 1) against seeded fixtures. + +Reproduce (the queries harness reads `DATABASE_URL` from `.env`): + +``` +cargo run --release --features bench --example bench_round23_micro +cargo run --release --features bench --example bench_round23_queries +``` + +## Summary + +| # | change | metric | before → after | +|--:|---|---|---| +| **J1** | `contact_pg_repository::row_to_contact` (+ the inlined `contact_group_pg_repository` sibling) decoded each of the 3 JSONB columns (`email`/`phone`/`address`) with `row.get::` + `serde_json::from_value::>` — a throwaway `Value` DOM built per column and then walked a **second** time to produce the typed `Vec`. Now `row.try_get::>>` decodes the JSONB bytes straight into the typed Vec in one `from_slice` pass, no DOM. Runs **per contact row** of every contact list / multiget / CardDAV sync. | micro allocs · PG p50 | **84 → 33 allocs/op** (2.15× wall) · **3794 → 2360 ns/contact** (1.61×) | +| **J2** | `DrivePolicies::from_value` did `serde_json::from_value(value.clone())` — cloning the **entire** policies `Value` DOM before walking it, on every drive-policy read (move/copy, shared-link creation, grant). Now `DrivePolicies::deserialize(value)` deserializes straight from the borrow (serde_json's `Deserializer for &Value`), no clone — a one-line body change, byte-identical, all 7 call sites unchanged. | micro allocs | **5 → 0 allocs/op** (11.51× wall) | +| **P1** | `AuthApplicationService::get_user_profile` issued two **independent, serial** `get_user_by_id` point reads (caller then target; the self-case short-circuit compares input UUIDs, not fetched data). Now the self-case does a single fetch and the non-self path overlaps caller+target with `tokio::join!` (`caller_res?` first preserves the caller-error precedence). | PG p50 | **577 → 312 µs/call** (1.85×) | +| **G1** | `SubjectGroupService::remove_member` ran the child group's transitive-user recursive CTE **twice** for a nested `Group` removal — once in the would-empty pre-check, once in `invalidation_targets` after the remove. The edge delete is *above* the child, so its descendants can't change; now the CTE runs **once** and the result is reused for both. | PG p50 | **829 → 412 µs/removal** (2.01×) | +| **U1** | `dedup_service` (`store_loose_chunks` final registration + the ingest `run_rollback`) built `Vec`/`Vec` by **cloning** every 64-byte hash out of an owned, dead-after `Vec<(String,i64)>` purely to reshape for `sync_blobs(&[String])` + the `UNNEST` bind. Now `into_iter().unzip()` moves the hashes out — no per-hash content copy. | micro allocs | **256 → 0 hash clones** (1283 → 1027 allocs/op on a 256-chunk batch) | + +> The micro allocs/op is the deterministic gate (identical run to run); the PG +> p50 is single-machine, warm-pool, and noise-bounded. Every section carries a +> value-equivalence gate; the shipped source matches each AFTER arm. + +## [J1] Contact JSONB — typed `Json` decode, no intermediate `Value` DOM + +`row_to_contact` (reached by 11 call sites — every contact GET / list / +paginated list / multiget / CardDAV cursor stream / search / by-email / +by-group / create+update RETURNING) and the identical inlined block in +`contact_group_pg_repository::get_contacts_in_group` both did: + +```rust +let email_json: JsonValue = row.get("email"); // sqlx JSONB → Value DOM (alloc tree) +let emails = serde_json::from_value::>(email_json) // walk the DOM again + .map(emails_from_persistence).unwrap_or_default(); +// … same for phone, address +``` + +`sqlx::types::Json` decodes the raw JSONB bytes with a single +`serde_json::from_slice::` (sqlx-core 0.8.6 `types/json.rs`), skipping the +`Value` tree entirely: + +```rust +let emails = row + .try_get::>, _>("email") + .map(|j| emails_from_persistence(j.0)) + .unwrap_or_default(); +``` + +`try_get` (not `get`) preserves the exact malformed-shape fallback — `get` +would panic on a decode error, whereas the old `from_value(...).unwrap_or_default()` +tolerated it. The columns are `JSONB NOT NULL DEFAULT '[]'`, so SQL NULL never +occurs. Byte-identical: both paths run the same derived `Deserialize>` +over the same bytes — the `bench_round23_queries` §Q1 gate asserts the two +decode the 500 seeded contacts field-for-field identically. The micro shows the +3 discarded DOMs/row (84 → 33 allocs); on the real rows the decode is 1.61×. + +## [J2] Drive policies — deserialize from the borrow, don't clone the DOM + +`DrivePolicies::from_value(value: &serde_json::Value)` is called on every +drive-policy read (`get_policies_for_file/_folder`, +`get_drive_id_and_policies_for_*`, `update_policies` RETURNING, the ACL engine's +enforcement read, and `Drive::typed_policies`). It built the typed struct with +`serde_json::from_value(value.clone())` — a full clone of the policies DOM +purely because `from_value` consumes its argument. serde_json implements +`Deserializer` for `&Value`, so the struct can be built straight from the +borrow: + +```rust +use serde::Deserialize as _; +Self::deserialize(value).unwrap_or_default() // was: serde_json::from_value(value.clone()) +``` + +Byte-identical (same derived `Deserialize`, same lenient `unwrap_or_default` +fallback that keeps unknown keys on disk), a one-line body change, and every +caller keeps its `&Value` argument unchanged — so `typed_policies(&self)` +(which only has a borrow of `self.policies`) also stops cloning. The micro +(a realistic bag with a preserved unknown key) drops 5 → 0 allocs/op. + +## [P1] `get_user_profile` — overlap the two independent reads with `join!` + +The profile lookup fetched the caller and the target user in two serial +round-trips. The self-case (`caller_id == target_id`) is decided by comparing +the **input** UUIDs, so on the common non-self path the two reads are +independent — query 2 never depends on query 1. AFTER: + +```rust +if caller_id == target_id { // self: one fetch, unchanged + let caller = self.user_storage.get_user_by_id(caller_id).await?; + return Ok(UserDto::from(caller)); +} +let (caller_res, target_res) = tokio::join!( // non-self: overlap + self.user_storage.get_user_by_id(caller_id), + self.user_storage.get_user_by_id(target_id)); +let caller = caller_res?; // caller-error precedence preserved +let target = match target_res { … }; // identical NotFound→anonymized-404 + audit +``` + +Every observable outcome is preserved (self still 1 fetch, the anti-enumeration +audit unchanged). The §Q4 gate asserts identical ids from both shapes; two +warm-pool serial reads vs the `join!` measured **1.85×**. + +## [G1] `remove_member` — compute the child's transitive users once, reuse it + +For a nested `GroupMember::Group(child_id)` removal the child's transitive-user +set (a recursive `WITH RECURSIVE` CTE over `subject_group_members`) was computed +**twice**: once in the would-empty self-defense pre-check, and again inside +`invalidation_targets` after `remove_member` deleted the parent→child edge. That +edge is *above* the child, so the child's own descendants are unchanged — +verified empirically on the live DB (child set `{u2,u3}` identical before and +after the edge delete). AFTER computes the CTE once, up front, and reuses it for +both the pre-check and the cache-invalidation set (`invalidation_targets` stays +for `add_member`). The §Q6 gate asserts the child set is both stable and the +expected `{u2,u3}`; 2 CTEs vs 1 measured **2.01×** on the seeded 3-level tree. + +## [U1] dedup hash reshape — move via `unzip`, don't clone + +`store_loose_chunks`'s final registration and the ingest `run_rollback` both +reshaped an owned `Vec<(String,i64)>` (dead after the block) into the +`Vec` + `Vec` that `sync_blobs(&[String])` and the `UNNEST` bind +need, by cloning every 64-char hash: + +```rust +let hashes: Vec = new_rows.iter().map(|(h, _)| h.clone()).collect(); // N clones +let sizes: Vec = new_rows.iter().map(|(_, s)| *s).collect(); +``` + +Since the source is owned and never read again, `into_iter().unzip()` moves the +hashes out — 0 per-hash content copies: + +```rust +let (hashes, sizes): (Vec, Vec) = new_rows.into_iter().unzip(); +``` + +Byte-identical rows inserted; the micro (256 distinct new chunks) drops exactly +the 256 hash clones. (This is the move-not-borrow refinement of the ROUND21 §R2 +`&[&str]` pattern — `sync_blobs` takes `&[String]`, so a borrow would force a +port-signature change across 6 backends, whereas the move needs none.) + +## Not shipped — deferred to a dedicated pass + +- **`batch_operations::download_zip` per-item N+1** (the audit's #2, highest + raw-latency candidate): the file loop calls `get_file_with_perms` (itself + authz + `get_file` = 2 round-trips) per selected file, then + `add_file_entry_streamed` — which **re-authorizes** internally via + `get_file_stream_with_perms`. Collapsing the per-item metadata+authz into a + bulk `get_files_by_ids` + `check_files_read_batch` prefetch is a real win + (`2N+2M` serial round-trips → ~3 batch queries), **but** it moves the sole + authorization from before the stream to inside it, so it needs a careful + AuthZ-ordering + anti-enumeration proof (the project's rule: authz lives in + the service layer, denials audit-log and return the anti-enum shape). That is + its own validated pass, not a perf banner — queued with a `download_zip` + fixture that seeds a large multi-select and asserts identical ZIP entry + set+order across the change. +- **Contact/Drive JSONB — the SQL-NULL edge**: the typed `try_get`/`deserialize` + paths return the empty/default on SQL NULL where the old `row.get::` + would have panicked. Both columns are `NOT NULL DEFAULT` today so this never + fires; noted only so a future nullable-column change re-checks it. + +## Environment / methodology + +- A local **PostgreSQL 16** dev instance was provisioned for this round + (schema applied via the 67 `migrations/*.sql` in order; `pg_trgm` + `ltree` + extensions). `bench_round23_queries` seeds its own fixtures (unique + `bench23-*` markers) and tears them down (idempotent cleanup) around the run. +- **Build note:** this session's host intermittently `SIGILL`ed rustc/LLVM + codegen under the repo's default `-C target-cpu=native` (a `cascadelake` with + AVX-512 whose passthrough faulted after a host migration). All Round-23 + builds/benches were run with `RUSTFLAGS="-C target-cpu=x86-64-v3"` (AVX2, no + AVX-512) to sidestep it. This is a local build-flag override only — the + checked-in `.cargo/config.toml` is unchanged, and the primary gate (allocs/op) + is target-cpu-independent; the PG p50 comparisons use the same flag for both + arms, so the relative speedups hold. +- Each micro section: BEFORE (verbatim shipped-before shape) vs AFTER (verbatim + shipped-after shape) + a value-equivalence assert + a `GATE FAIL … rollback` + exit if the AFTER fails to reduce allocations. Each PG section: BEFORE vs + AFTER shape against real seeded rows + an equivalence gate (mismatch → exit 1) + + p50 over `BENCH_PASSES`. +- Verified beyond the benches: `cargo fmt --all --check` clean, `cargo clippy + --features bench --all-targets -D warnings` clean, and the touched modules' + unit tests pass (`contact`, `drive`, `subject_group`, `dedup`, auth profile). diff --git a/examples/bench_round23_micro.rs b/examples/bench_round23_micro.rs new file mode 100644 index 00000000..d018ab1c --- /dev/null +++ b/examples/bench_round23_micro.rs @@ -0,0 +1,363 @@ +//! Round-23 CPU/alloc micro-pack (no Postgres) — the deterministic alloc gates +//! for the decode / clone candidates. The end-to-end PostgreSQL latency + +//! equivalence evidence lives in `bench_round23_queries.rs`. +//! +//! Same rule as ROUND2–22: each section is BEFORE (verbatim replica of the +//! shipped-before shape) vs AFTER (verbatim replica of the shipped-after shape, +//! which the source is then made to match), with a byte/-value equivalence gate +//! and a `GATE FAIL … rollback` check that `std::process::exit(1)`s if the AFTER +//! arm fails to beat its BEFORE. +//! +//! [J1] `contact_pg_repository::row_to_contact` (+ the `contact_group` +//! sibling) decoded each JSONB column with `row.get::` +//! + `serde_json::from_value::>` — a throwaway `Value` DOM per +//! column, walked a second time. AFTER decodes straight into the typed +//! Vec via `sqlx::types::Json` (one `from_slice` pass). Modeled here +//! as `from_slice::` + `from_value` vs `from_slice::>`. +//! +//! [J2] `DrivePolicies::from_value` did `serde_json::from_value(value.clone())` +//! — cloning the ENTIRE policies DOM per drive-policy read. AFTER +//! deserializes from the borrow (`T::deserialize(&Value)`), no clone. +//! +//! [U1] `dedup_service` (`store_loose_chunks` final registration + the ingest +//! `run_rollback`) built `Vec`/`Vec` by CLONING every hash +//! out of an owned, dead-after `Vec<(String,i64)>` purely to reshape for +//! `sync_blobs(&[String])` + the UNNEST bind. AFTER moves via +//! `into_iter().unzip()`. +//! +//! Run: +//! cargo run --release --features bench --example bench_round23_micro +//! Tunables (env): BENCH_ITERS (200000), J1_ROWS (3), U1_CHUNKS (256) + +use std::alloc::{GlobalAlloc, Layout, System}; +use std::env; +use std::hint::black_box; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::time::Instant; + +use serde::{Deserialize, Serialize}; +use serde_json::Value; + +static ALLOC_CALLS: AtomicU64 = AtomicU64::new(0); + +struct CountingAlloc; + +unsafe impl GlobalAlloc for CountingAlloc { + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + ALLOC_CALLS.fetch_add(1, Ordering::Relaxed); + unsafe { System.alloc(layout) } + } + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + unsafe { System.dealloc(ptr, layout) } + } + unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { + ALLOC_CALLS.fetch_add(1, Ordering::Relaxed); + unsafe { System.realloc(ptr, layout, new_size) } + } + unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { + ALLOC_CALLS.fetch_add(1, Ordering::Relaxed); + unsafe { System.alloc_zeroed(layout) } + } +} + +#[global_allocator] +static GLOBAL: CountingAlloc = CountingAlloc; + +fn env_or(key: &str, default: T) -> T { + env::var(key) + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(default) +} + +struct Measured { + wall_ns_per_op: f64, + allocs_per_op: f64, +} + +fn measure(iters: usize, mut f: F) -> Measured { + for _ in 0..(iters / 20).max(1) { + f(); + } + let a0 = ALLOC_CALLS.load(Ordering::Relaxed); + let t = Instant::now(); + for _ in 0..iters { + f(); + } + let wall = t.elapsed().as_nanos() as f64 / iters as f64; + let allocs = (ALLOC_CALLS.load(Ordering::Relaxed) - a0) as f64 / iters as f64; + Measured { + wall_ns_per_op: wall, + allocs_per_op: allocs, + } +} + +fn print_row(label: &str, m: &Measured) { + println!( + "| {:<52} | {:>12.1} | {:>10.2} |", + label, m.wall_ns_per_op, m.allocs_per_op + ); +} + +fn header_footer(name: &str, before: &Measured, after: &Measured) { + println!("| arm | ns/op | allocs/op |"); + print_row(&format!("BEFORE {name}"), before); + print_row(&format!("AFTER {name}"), after); + println!( + "# {:.2}x wall, {:.2} fewer allocs/op", + before.wall_ns_per_op / after.wall_ns_per_op, + before.allocs_per_op - after.allocs_per_op + ); +} + +fn gate_allocs(tag: &str, before: &Measured, after: &Measured) { + if after.allocs_per_op >= before.allocs_per_op { + eprintln!("GATE FAIL [{tag}]: AFTER did not reduce allocations — rollback"); + std::process::exit(1); + } +} + +// ──────────────────────────────────────────────────────────────────────────── +// [J1] Contact JSONB decode — Value DOM + from_value vs Json from_slice. +// Verbatim replicas of the persistence DTOs (contact_persistence_dto.rs). +// ──────────────────────────────────────────────────────────────────────────── + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +struct EmailDto { + email: String, + r#type: String, + is_primary: bool, +} +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +struct PhoneDto { + number: String, + r#type: String, + is_primary: bool, +} +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +struct AddressDto { + street: Option, + city: Option, + state: Option, + postal_code: Option, + country: Option, + r#type: String, + is_primary: bool, +} + +/// BEFORE: `row.get::` (sqlx JSONB→Value DOM) then `from_value::>` +/// (a second walk of the DOM). Modeled with `from_slice::` (what sqlx's +/// Value decoder does) + `from_value`. +fn j1_before( + email: &[u8], + phone: &[u8], + addr: &[u8], +) -> (Vec, Vec, Vec) { + let ev: Value = serde_json::from_slice(email).unwrap(); + let pv: Value = serde_json::from_slice(phone).unwrap(); + let av: Value = serde_json::from_slice(addr).unwrap(); + let emails = serde_json::from_value::>(ev).unwrap_or_default(); + let phones = serde_json::from_value::>(pv).unwrap_or_default(); + let addrs = serde_json::from_value::>(av).unwrap_or_default(); + (emails, phones, addrs) +} + +/// AFTER: `sqlx::types::Json>` decodes the JSONB bytes straight into the +/// typed Vec (one `from_slice::>`), no intermediate DOM. +fn j1_after( + email: &[u8], + phone: &[u8], + addr: &[u8], +) -> (Vec, Vec, Vec) { + let emails = serde_json::from_slice::>(email).unwrap_or_default(); + let phones = serde_json::from_slice::>(phone).unwrap_or_default(); + let addrs = serde_json::from_slice::>(addr).unwrap_or_default(); + (emails, phones, addrs) +} + +fn section_j1() { + let iters: usize = env_or("BENCH_ITERS", 200_000); + let n: usize = env_or("J1_ROWS", 3); // entries per column, realistic contact + + let mk_emails = |n: usize| -> Vec { + (0..n) + .map(|i| EmailDto { + email: format!("user{i}@example.com"), + r#type: if i == 0 { "home" } else { "work" }.to_string(), + is_primary: i == 0, + }) + .collect() + }; + let mk_phones = |n: usize| -> Vec { + (0..n) + .map(|i| PhoneDto { + number: format!("+1-555-010{i}"), + r#type: "cell".to_string(), + is_primary: i == 0, + }) + .collect() + }; + let mk_addrs = |n: usize| -> Vec { + (0..n) + .map(|i| AddressDto { + street: Some(format!("{} Main St", 100 + i)), + city: Some("Springfield".to_string()), + state: Some("IL".to_string()), + postal_code: Some("62704".to_string()), + country: Some("US".to_string()), + r#type: "home".to_string(), + is_primary: i == 0, + }) + .collect() + }; + + let email_b = serde_json::to_vec(&mk_emails(n)).unwrap(); + let phone_b = serde_json::to_vec(&mk_phones(n)).unwrap(); + let addr_b = serde_json::to_vec(&mk_addrs(n)).unwrap(); + + // Equivalence: identical decoded Vecs. + assert_eq!( + j1_before(&email_b, &phone_b, &addr_b), + j1_after(&email_b, &phone_b, &addr_b), + "J1 decoded contacts differ" + ); + + let before = measure(iters, || { + black_box(j1_before( + black_box(&email_b), + black_box(&phone_b), + black_box(&addr_b), + )); + }); + let after = measure(iters, || { + black_box(j1_after( + black_box(&email_b), + black_box(&phone_b), + black_box(&addr_b), + )); + }); + + println!( + "\n## [J1] Contact JSONB decode ({n} entries/col — per contact row of every list/multiget/sync)" + ); + header_footer( + "Value DOM + from_value vs Json from_slice", + &before, + &after, + ); + gate_allocs("J1", &before, &after); +} + +// ──────────────────────────────────────────────────────────────────────────── +// [J2] Drive policies decode — from_value(value.clone()) vs deserialize(&value). +// ──────────────────────────────────────────────────────────────────────────── + +#[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)] +#[serde(default)] +struct Policies { + forbid_public_links: bool, + read_only: bool, +} + +/// BEFORE: clone the whole `Value` DOM, then `from_value`. +fn j2_before(value: &Value) -> Policies { + serde_json::from_value(value.clone()).unwrap_or_default() +} + +/// AFTER: deserialize straight from the borrow — no DOM clone. +fn j2_after(value: &Value) -> Policies { + Policies::deserialize(value).unwrap_or_default() +} + +fn section_j2() { + let iters: usize = env_or("BENCH_ITERS", 200_000); + // A realistic on-disk policies bag with an unknown key preserved on disk + // (the lenient contract) so the DOM isn't trivially tiny. + let value: Value = serde_json::from_str( + r#"{"forbid_public_links":true,"read_only":false,"x_future_flag":"kept-on-disk"}"#, + ) + .unwrap(); + + assert_eq!( + j2_before(&value), + j2_after(&value), + "J2 decoded policies differ" + ); + assert!(j2_after(&value).forbid_public_links); + + let before = measure(iters, || { + black_box(j2_before(black_box(&value))); + }); + let after = measure(iters, || { + black_box(j2_after(black_box(&value))); + }); + + println!("\n## [J2] Drive policies decode (per move/copy/share/grant drive-policy read)"); + header_footer( + "from_value(value.clone()) vs deserialize(&value)", + &before, + &after, + ); + gate_allocs("J2", &before, &after); +} + +// ──────────────────────────────────────────────────────────────────────────── +// [U1] dedup hash reshape — clone-collect vs into_iter().unzip(). +// ──────────────────────────────────────────────────────────────────────────── + +fn u1_build(n: usize) -> Vec<(String, i64)> { + (0..n) + .map(|i| { + ( + format!("{:064x}", i as u128 * 0x9E37_79B9_7F4A_7C15), + i as i64, + ) + }) + .collect() +} + +/// BEFORE: clone every hash out of the owned (dead-after) Vec to reshape. +fn u1_before(rows: Vec<(String, i64)>) -> (Vec, Vec) { + let hashes: Vec = rows.iter().map(|(h, _)| h.clone()).collect(); + let sizes: Vec = rows.iter().map(|(_, s)| *s).collect(); + (hashes, sizes) +} + +/// AFTER: move via unzip — no per-hash content copy. +fn u1_after(rows: Vec<(String, i64)>) -> (Vec, Vec) { + rows.into_iter().unzip() +} + +fn section_u1() { + let n: usize = env_or("U1_CHUNKS", 256); + let iters: usize = env_or("BENCH_ITERS", 200_000) / 20; // heavier op + + // Equivalence: identical hashes + sizes. + assert_eq!( + u1_before(u1_build(n)), + u1_after(u1_build(n)), + "U1 reshape differs" + ); + + let before = measure(iters, || { + black_box(u1_before(black_box(u1_build(n)))); + }); + let after = measure(iters, || { + black_box(u1_after(black_box(u1_build(n)))); + }); + + println!( + "\n## [U1] dedup hash reshape ({n} distinct new chunks — per delta-upload registration)" + ); + header_footer("clone-collect vs into_iter().unzip()", &before, &after); + gate_allocs("U1", &before, &after); +} + +fn main() { + println!("# Round-23 micro-pack — BEFORE/AFTER (counting allocator, release)"); + println!("# allocs/op is the deterministic gate; a non-winning AFTER exits 1 (rollback)."); + section_j1(); + section_j2(); + section_u1(); + println!("\nAll Round-23 micro sections passed their allocation gate."); +} diff --git a/examples/bench_round23_queries.rs b/examples/bench_round23_queries.rs new file mode 100644 index 00000000..2ac2f590 --- /dev/null +++ b/examples/bench_round23_queries.rs @@ -0,0 +1,433 @@ +//! Round-23 PostgreSQL query-shape pack — end-to-end latency + equivalence on +//! the live dev Postgres. The deterministic alloc gates for the decode/clone +//! candidates live in `bench_round23_micro.rs`; this harness measures the real +//! round-trip / decode wins against seeded fixtures and asserts identical +//! results (the equivalence gate — a mismatch `std::process::exit(1)`s). +//! +//! [Q1] Contact JSONB decode on REAL rows (contact_pg §J1): fetch a seeded +//! address book's contacts once, then decode the `email`/`phone`/`address` +//! JSONB columns BEFORE (`row.get::` + `from_value`) vs AFTER +//! (`row.try_get::>>`). Gate: identical decode. +//! +//! [Q4] `get_user_profile` (§P1): two independent point reads of the caller + +//! target users, BEFORE serial (`await` then `await`) vs AFTER concurrent +//! (`tokio::join!`). Gate: identical rows. +//! +//! [Q6] `subject_group::remove_member` (§G1): the child group's transitive +//! user set (a recursive CTE) BEFORE computed TWICE (the shipped-before +//! pre-check + `invalidation_targets`) vs AFTER once + reused. Gate: +//! identical user set. +//! +//! Run (needs the dev Postgres up; reads DATABASE_URL from .env): +//! cargo run --release --features bench --example bench_round23_queries +//! Tunables (env): BENCH_PASSES (200), Q1_CONTACTS (500), Q1_DECODE_PASSES (4000) + +use std::env; +use std::time::Instant; + +use serde::{Deserialize, Serialize}; +use serde_json::Value; +use sqlx::postgres::PgPoolOptions; +use sqlx::{PgPool, Row}; +use uuid::Uuid; + +fn env_or(key: &str, default: T) -> T { + env::var(key) + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(default) +} + +fn p50(mut samples: Vec) -> f64 { + samples.sort_by(|a, b| a.partial_cmp(b).unwrap()); + samples[samples.len() / 2] +} + +fn report(tag: &str, unit: &str, before: f64, after: f64) { + println!( + "| {:<44} | {:>12} | {:>12} | {:>7} |", + tag, "BEFORE", "AFTER", "speedup" + ); + println!( + "| {:<44} | {:>12.1} | {:>12.1} | {:>6.2}x |", + unit, + before, + after, + before / after + ); +} + +// ── Verbatim replicas of contact_persistence_dto.rs ────────────────────────── +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +struct EmailDto { + email: String, + r#type: String, + is_primary: bool, +} +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +struct PhoneDto { + number: String, + r#type: String, + is_primary: bool, +} +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +struct AddressDto { + street: Option, + city: Option, + state: Option, + postal_code: Option, + country: Option, + r#type: String, + is_primary: bool, +} + +async fn cleanup(pool: &PgPool) { + // Idempotent teardown (also clears any fixtures a prior crashed run left). + // Memberships first (FK to both groups and users), targeted by the bench + // group names so it catches them whoever `added_by` is. + let _ = sqlx::query( + "DELETE FROM auth.subject_group_members WHERE group_id IN + (SELECT id FROM auth.subject_groups + WHERE name IN ('bench23parent','bench23child','bench23grand'))", + ) + .execute(pool) + .await; + let _ = sqlx::query( + "DELETE FROM auth.subject_groups WHERE name IN ('bench23parent','bench23child','bench23grand')", + ) + .execute(pool) + .await; + let _ = sqlx::query("DELETE FROM carddav.contacts WHERE uid LIKE 'bench23-%'") + .execute(pool) + .await; + let _ = sqlx::query("DELETE FROM carddav.address_books WHERE name = 'bench23_ab'") + .execute(pool) + .await; + let _ = sqlx::query("DELETE FROM auth.users WHERE email LIKE 'bench23-%@bench.invalid'") + .execute(pool) + .await; +} + +async fn seed_user(pool: &PgPool, tag: &str) -> Uuid { + sqlx::query_scalar( + "INSERT INTO auth.users (username, email, role) + VALUES ($1, $2, 'user') RETURNING id", + ) + .bind(format!("bench23_{tag}")) + .bind(format!("bench23-{tag}@bench.invalid")) + .fetch_one(pool) + .await + .expect("seed user") +} + +// ── [Q1] Contact JSONB decode ──────────────────────────────────────────────── +async fn section_q1(pool: &PgPool) { + let n: usize = env_or("Q1_CONTACTS", 500); + let passes: usize = env_or("Q1_DECODE_PASSES", 4000); + + let owner = seed_user(pool, "q1owner").await; + let ab: Uuid = sqlx::query_scalar( + "INSERT INTO carddav.address_books (id, name, owner_id) + VALUES (gen_random_uuid(), 'bench23_ab', $1) RETURNING id", + ) + .bind(owner) + .fetch_one(pool) + .await + .expect("seed address book"); + + for i in 0..n { + let emails = serde_json::to_value(vec![ + EmailDto { + email: format!("user{i}@example.com"), + r#type: "home".into(), + is_primary: true, + }, + EmailDto { + email: format!("user{i}@work.example.com"), + r#type: "work".into(), + is_primary: false, + }, + ]) + .unwrap(); + let phones = serde_json::to_value(vec![PhoneDto { + number: format!("+1-555-01{i:04}"), + r#type: "cell".into(), + is_primary: true, + }]) + .unwrap(); + let addrs = serde_json::to_value(vec![AddressDto { + street: Some(format!("{} Main St", 100 + i)), + city: Some("Springfield".into()), + state: Some("IL".into()), + postal_code: Some("62704".into()), + country: Some("US".into()), + r#type: "home".into(), + is_primary: true, + }]) + .unwrap(); + sqlx::query( + "INSERT INTO carddav.contacts (id, address_book_id, uid, full_name, email, phone, address, etag) + VALUES (gen_random_uuid(), $1, $2, $3, $4, $5, $6, $7)", + ) + .bind(ab) + .bind(format!("bench23-{i}")) + .bind(format!("Contact {i}")) + .bind(&emails) + .bind(&phones) + .bind(&addrs) + .bind(format!("etag-{i}")) + .execute(pool) + .await + .expect("seed contact"); + } + + // Fetch the rows ONCE (the query round-trip is out of the measured window — + // we isolate the per-row decode, which is what §J1 changes). + let rows = sqlx::query( + "SELECT email, phone, address FROM carddav.contacts + WHERE address_book_id = $1 ORDER BY uid", + ) + .bind(ab) + .fetch_all(pool) + .await + .expect("fetch contacts"); + assert_eq!(rows.len(), n, "Q1 seeded row count"); + + // BEFORE: Value DOM + from_value per column. + let decode_before = + |rows: &[sqlx::postgres::PgRow]| -> Vec<(Vec, Vec, Vec)> { + rows.iter() + .map(|r| { + let ev: Value = r.get("email"); + let pv: Value = r.get("phone"); + let av: Value = r.get("address"); + ( + serde_json::from_value::>(ev).unwrap_or_default(), + serde_json::from_value::>(pv).unwrap_or_default(), + serde_json::from_value::>(av).unwrap_or_default(), + ) + }) + .collect() + }; + // AFTER: typed Json decode straight from the JSONB bytes. + let decode_after = + |rows: &[sqlx::postgres::PgRow]| -> Vec<(Vec, Vec, Vec)> { + rows.iter() + .map(|r| { + ( + r.try_get::>, _>("email") + .map(|j| j.0) + .unwrap_or_default(), + r.try_get::>, _>("phone") + .map(|j| j.0) + .unwrap_or_default(), + r.try_get::>, _>("address") + .map(|j| j.0) + .unwrap_or_default(), + ) + }) + .collect() + }; + + // Equivalence gate. + if decode_before(&rows) != decode_after(&rows) { + eprintln!("GATE FAIL [Q1]: BEFORE/AFTER decode differ — rollback"); + cleanup(pool).await; + std::process::exit(1); + } + + let mut b = Vec::with_capacity(passes); + let mut a = Vec::with_capacity(passes); + for _ in 0..passes / 20 { + std::hint::black_box(decode_before(&rows)); + std::hint::black_box(decode_after(&rows)); + } + for _ in 0..passes { + let t = Instant::now(); + std::hint::black_box(decode_before(&rows)); + b.push(t.elapsed().as_nanos() as f64 / n as f64); + let t = Instant::now(); + std::hint::black_box(decode_after(&rows)); + a.push(t.elapsed().as_nanos() as f64 / n as f64); + } + + println!( + "\n## [Q1] Contact JSONB decode on real rows ({n} contacts) — gate OK (identical decode)" + ); + report( + "Value DOM + from_value vs Json", + "p50 ns/contact", + p50(b), + p50(a), + ); +} + +// ── [Q4] get_user_profile: serial vs join! ────────────────────────────────── +async fn section_q4(pool: &PgPool) { + let passes: usize = env_or("BENCH_PASSES", 200); + let caller = seed_user(pool, "q4caller").await; + let target = seed_user(pool, "q4target").await; + + // Capture `pool` (not take it as a param) so the returned future borrows a + // single concrete lifetime — a closure param `&PgPool` + future return hits + // the HRTB limitation. + let read = |id: Uuid| async move { + sqlx::query("SELECT id, email, role FROM auth.users WHERE id = $1") + .bind(id) + .fetch_optional(pool) + .await + .expect("read user") + .map(|r| r.get::("id")) + }; + + // Equivalence gate: same two ids either way. + let ser = (read(caller).await, read(target).await); + let (jc, jt) = tokio::join!(read(caller), read(target)); + if ser != (jc, jt) { + eprintln!("GATE FAIL [Q4]: serial/join ids differ — rollback"); + cleanup(pool).await; + std::process::exit(1); + } + + let mut b = Vec::with_capacity(passes); + let mut a = Vec::with_capacity(passes); + for _ in 0..(passes / 20).max(1) { + let _ = (read(caller).await, read(target).await); + let _ = tokio::join!(read(caller), read(target)); + } + for _ in 0..passes { + let t = Instant::now(); + let _ = std::hint::black_box((read(caller).await, read(target).await)); + b.push(t.elapsed().as_nanos() as f64); + let t = Instant::now(); + let _ = std::hint::black_box(tokio::join!(read(caller), read(target))); + a.push(t.elapsed().as_nanos() as f64); + } + + println!("\n## [Q4] get_user_profile caller+target reads — gate OK (identical ids)"); + report( + "2 serial reads vs tokio::join!", + "p50 ns/call", + p50(b), + p50(a), + ); +} + +// ── [Q6] subject_group child transitive users: 2 CTEs vs 1 ─────────────────── +async fn section_q6(pool: &PgPool) { + let passes: usize = env_or("BENCH_PASSES", 200); + // Tree: parent → child → {grandchild, u2}; grandchild → u3. u1 direct on parent. + let u1 = seed_user(pool, "q6u1").await; + let u2 = seed_user(pool, "q6u2").await; + let u3 = seed_user(pool, "q6u3").await; + let mk_group = |name: &'static str| async move { + sqlx::query_scalar::<_, Uuid>( + "INSERT INTO auth.subject_groups (name) VALUES ($1) RETURNING id", + ) + .bind(name) + .fetch_one(pool) + .await + .expect("seed group") + }; + let parent = mk_group("bench23parent").await; + let child = mk_group("bench23child").await; + let grand = mk_group("bench23grand").await; + let add_ug = |g: Uuid, u: Uuid| async move { + sqlx::query("INSERT INTO auth.subject_group_members (group_id, member_user_id, added_by) VALUES ($1, $2, $3)") + .bind(g).bind(u).bind(u1).execute(pool).await.expect("add user member"); + }; + let add_gg = |g: Uuid, c: Uuid| async move { + sqlx::query("INSERT INTO auth.subject_group_members (group_id, member_group_id, added_by) VALUES ($1, $2, $3)") + .bind(g).bind(c).bind(u1).execute(pool).await.expect("add group member"); + }; + add_ug(parent, u1).await; + add_gg(parent, child).await; + add_gg(child, grand).await; + add_ug(child, u2).await; + add_ug(grand, u3).await; + + let cte = |gid: Uuid| async move { + let rows = sqlx::query( + "WITH RECURSIVE descendants AS ( + SELECT $1::uuid AS g + UNION + SELECT m.member_group_id FROM auth.subject_group_members m + JOIN descendants d ON m.group_id = d.g WHERE m.member_group_id IS NOT NULL) + SELECT DISTINCT m.member_user_id AS user_id FROM auth.subject_group_members m + JOIN descendants d ON m.group_id = d.g WHERE m.member_user_id IS NOT NULL", + ) + .bind(gid) + .fetch_all(pool) + .await + .expect("cte"); + let mut ids: Vec = rows.iter().map(|r| r.get::("user_id")).collect(); + ids.sort(); + ids + }; + + // Equivalence: the child's transitive set is {u2, u3}, and it is IDENTICAL + // whether computed once or twice (the edge delete above the child cannot + // change its descendants — the §G1 correctness claim). + let once = cte(child).await; + let twice = { + let _first = cte(child).await; + cte(child).await + }; + let mut expected = [u2, u3]; + expected.sort(); + if once != twice || once != expected { + eprintln!("GATE FAIL [Q6]: child transitive set not stable/expected — rollback"); + cleanup(pool).await; + std::process::exit(1); + } + + let mut b = Vec::with_capacity(passes); + let mut a = Vec::with_capacity(passes); + for _ in 0..(passes / 20).max(1) { + let _ = (cte(child).await, cte(child).await); + let _ = cte(child).await; + } + for _ in 0..passes { + // BEFORE: the child CTE runs TWICE (pre-check + invalidation_targets). + let t = Instant::now(); + let _ = cte(child).await; + let _ = std::hint::black_box(cte(child).await); + b.push(t.elapsed().as_nanos() as f64); + // AFTER: once, reused. + let t = Instant::now(); + let _ = std::hint::black_box(cte(child).await); + a.push(t.elapsed().as_nanos() as f64); + } + + println!("\n## [Q6] subject_group child transitive users — gate OK (stable set {{u2,u3}})"); + report( + "2 recursive CTEs vs 1 (reused)", + "p50 ns/removal", + p50(b), + p50(a), + ); +} + +#[tokio::main(flavor = "multi_thread")] +async fn main() { + dotenvy::dotenv().ok(); + let url = env::var("DATABASE_URL") + .or_else(|_| env::var("OXICLOUD_DB_CONNECTION_STRING")) + .expect("set DATABASE_URL — the dev Postgres URL"); + let pool = PgPoolOptions::new() + .max_connections(8) + .connect(&url) + .await + .expect("connect Postgres"); + + println!("# Round-23 PG query-shape pack — BEFORE/AFTER (live Postgres)"); + println!("# Each section asserts an equivalence gate (mismatch → exit 1) and reports p50."); + + cleanup(&pool).await; + section_q1(&pool).await; + section_q4(&pool).await; + section_q6(&pool).await; + cleanup(&pool).await; + + println!("\nAll Round-23 query sections passed their equivalence gate."); +} diff --git a/src/application/services/auth_application_service.rs b/src/application/services/auth_application_service.rs index 373d8810..87befc00 100644 --- a/src/application/services/auth_application_service.rs +++ b/src/application/services/auth_application_service.rs @@ -1932,17 +1932,28 @@ impl AuthApplicationService { expose_system_users: bool, pool: &sqlx::PgPool, ) -> Result { - let caller = self.user_storage.get_user_by_id(caller_id).await?; - - // (1) Self. + // (1) Self — a single fetch suffices (the check compares the input + // UUIDs, so the target read is never needed on this path). if caller_id == target_id { + let caller = self.user_storage.get_user_by_id(caller_id).await?; return Ok(UserDto::from(caller)); } + // Caller and target are independent point reads (the self-case already + // returned; the branch above compares input UUIDs, not fetched data) — + // overlap them with `join!` instead of two serial round-trips. + // `caller_res?` first preserves the caller-error precedence of the old + // sequential form. (benches/ROUND23.md §P1) + let (caller_res, target_res) = tokio::join!( + self.user_storage.get_user_by_id(caller_id), + self.user_storage.get_user_by_id(target_id) + ); + let caller = caller_res?; + // Anti-enumeration: NotFound for everything that doesn't pass. // Convert a real NotFound on `target` to the same anonymous 404, // so existence isn't leaked through differential responses. - let target = match self.user_storage.get_user_by_id(target_id).await { + let target = match target_res { Ok(u) => u, Err(e) if e.kind == ErrorKind::NotFound => { tracing::info!( diff --git a/src/application/services/subject_group_service.rs b/src/application/services/subject_group_service.rs index 5c116b5d..180caf7c 100644 --- a/src/application/services/subject_group_service.rs +++ b/src/application/services/subject_group_service.rs @@ -477,6 +477,23 @@ impl SubjectGroupService { // user is still reachable via another path after this remove, // they stay in the set on the post-state, so the check would // pass on the next remove instead. + // For a nested child-group removal the child's transitive user set is + // needed twice: by the would-empty pre-check below AND, after the + // remove, as the cache-invalidation set. The edge delete is ABOVE the + // child, so it cannot change the child's descendants — compute the + // recursive CTE ONCE here and reuse it, instead of the identical query + // running twice (the second was hidden inside `invalidation_targets`). + // (benches/ROUND23.md §G1) + let child_users: Option> = match member { + GroupMember::Group(child_id) => Some( + self.repo + .list_transitive_users(child_id) + .await + .map_err(map_repo_err)?, + ), + GroupMember::User(_) => None, + }; + let users_before = self .repo .list_transitive_users(group_id) @@ -485,17 +502,12 @@ impl SubjectGroupService { if !users_before.is_empty() { let would_be_empty = match member { GroupMember::User(uid) => users_before.len() == 1 && users_before.contains(&uid), - GroupMember::Group(child_id) => { - // For child-group removal: would this drop the - // parent's transitive user set to 0? Look up the - // child's transitive users — if every user in the - // parent's set comes through the child, removing the - // child empties the parent. - let child_users = self - .repo - .list_transitive_users(child_id) - .await - .map_err(map_repo_err)?; + GroupMember::Group(_) => { + // Would removing this child drop the parent's transitive + // user set to 0? Reuse the child's transitive users + // computed above — if every user in the parent's set comes + // through the child, removing the child empties the parent. + let child_users = child_users.as_deref().unwrap_or(&[]); // Set probe instead of an O(|before|·|child|) slice scan // (benches/ROUND11.md §13: 5.7x at 500×500). let child_set: std::collections::HashSet<&uuid::Uuid> = @@ -535,7 +547,15 @@ impl SubjectGroupService { // ancestor. Without this, a removed-from-group user keeps // appearing as a transitive member in `expand_subject_for_listing` // for up to 30 s, surfacing grants they no longer have. - for uid in self.invalidation_targets(member).await? { + // + // Reuse the child's transitive users computed above (unchanged by the + // edge delete) as the invalidation set — no second recursive CTE. For a + // `User` member it's just that user. (benches/ROUND23.md §G1) + let invalidation: Vec = match member { + GroupMember::User(uid) => vec![uid], + GroupMember::Group(_) => child_users.unwrap_or_default(), + }; + for uid in invalidation { self.engine.invalidate_user_groups_cache(uid).await; self.drive_repo.invalidate_readable_for_user(uid).await; } diff --git a/src/domain/entities/drive.rs b/src/domain/entities/drive.rs index d5467d31..8b0bd95c 100644 --- a/src/domain/entities/drive.rs +++ b/src/domain/entities/drive.rs @@ -230,7 +230,15 @@ impl DrivePolicies { /// rather than refusing the read; enforcement code never panics on /// existing data. pub fn from_value(value: &serde_json::Value) -> Self { - serde_json::from_value(value.clone()).unwrap_or_default() + // Deserialize straight from the borrowed `Value` (`T::deserialize(&Value)`, + // via serde_json's `Deserializer for &Value`) instead of + // `serde_json::from_value(value.clone())` — the old form cloned the ENTIRE + // policies DOM before walking it, on every drive-policy read (move/copy, + // shared-link creation, grant). Byte-identical (same derived `Deserialize` + // impl); the lenient `unwrap_or_default` fallback is unchanged. + // (benches/ROUND23.md §J2) + use serde::Deserialize as _; + Self::deserialize(value).unwrap_or_default() } /// D5 `forbid_public_links` gate, used by every entry point that diff --git a/src/infrastructure/repositories/pg/contact_group_pg_repository.rs b/src/infrastructure/repositories/pg/contact_group_pg_repository.rs index fedc4db2..945d7d1e 100644 --- a/src/infrastructure/repositories/pg/contact_group_pg_repository.rs +++ b/src/infrastructure/repositories/pg/contact_group_pg_repository.rs @@ -1,5 +1,4 @@ use chrono::Utc; -use serde_json::Value as JsonValue; use sqlx::{PgPool, Row, types::Uuid}; use std::sync::Arc; @@ -220,18 +219,21 @@ impl ContactGroupRepository for ContactGroupPgRepository { let mut contacts = Vec::with_capacity(rows.len()); for row in &rows { - let email_json: JsonValue = row.get("email"); - let phone_json: JsonValue = row.get("phone"); - let address_json: JsonValue = row.get("address"); - - let emails = serde_json::from_value::>(email_json) - .map(emails_from_persistence) + // Typed `Json` decode (one `from_slice` pass) instead of the + // `Value` DOM + `from_value` re-walk — the contact_pg_repository + // §J1 fix applied to this inlined sibling. Byte-identical result, + // 3 fewer throwaway DOMs per contact. (benches/ROUND23.md §J1) + let emails = row + .try_get::>, _>("email") + .map(|j| emails_from_persistence(j.0)) .unwrap_or_default(); - let phones = serde_json::from_value::>(phone_json) - .map(phones_from_persistence) + let phones = row + .try_get::>, _>("phone") + .map(|j| phones_from_persistence(j.0)) .unwrap_or_default(); - let addresses = serde_json::from_value::>(address_json) - .map(addresses_from_persistence) + let addresses = row + .try_get::>, _>("address") + .map(|j| addresses_from_persistence(j.0)) .unwrap_or_default(); contacts.push(Contact::from_raw( diff --git a/src/infrastructure/repositories/pg/contact_pg_repository.rs b/src/infrastructure/repositories/pg/contact_pg_repository.rs index 1edebbee..d6ab1e5a 100644 --- a/src/infrastructure/repositories/pg/contact_pg_repository.rs +++ b/src/infrastructure/repositories/pg/contact_pg_repository.rs @@ -23,18 +23,27 @@ impl ContactPgRepository { /// Maps a database row to a Contact domain entity fn row_to_contact(row: &sqlx::postgres::PgRow) -> Result { - let email_json: JsonValue = row.get("email"); - let phone_json: JsonValue = row.get("phone"); - let address_json: JsonValue = row.get("address"); - - let emails = serde_json::from_value::>(email_json) - .map(emails_from_persistence) + // Decode each JSONB column straight into its typed Vec via + // `sqlx::types::Json` (a single `serde_json::from_slice` pass over + // the raw JSONB bytes) instead of `row.get::` + + // `serde_json::from_value`, which built a throwaway `Value` DOM per + // column and then walked it a SECOND time to produce the typed Vec — + // 3 discarded DOMs per contact row on every list / multiget / CardDAV + // sync. `try_get` preserves the exact malformed-shape fallback (the old + // `from_value(...).unwrap_or_default()`; a bare `row.get` would panic on + // a decode error); the columns are `JSONB NOT NULL DEFAULT '[]'`, so SQL + // NULL never occurs. (benches/ROUND23.md §J1) + let emails = row + .try_get::>, _>("email") + .map(|j| emails_from_persistence(j.0)) .unwrap_or_default(); - let phones = serde_json::from_value::>(phone_json) - .map(phones_from_persistence) + let phones = row + .try_get::>, _>("phone") + .map(|j| phones_from_persistence(j.0)) .unwrap_or_default(); - let addresses = serde_json::from_value::>(address_json) - .map(addresses_from_persistence) + let addresses = row + .try_get::>, _>("address") + .map(|j| addresses_from_persistence(j.0)) .unwrap_or_default(); Ok(Contact::from_raw( diff --git a/src/infrastructure/services/dedup_service.rs b/src/infrastructure/services/dedup_service.rs index 258c4416..ef6604e9 100644 --- a/src/infrastructure/services/dedup_service.rs +++ b/src/infrastructure/services/dedup_service.rs @@ -209,8 +209,11 @@ impl IngestGuard { // sweep can reclaim the bytes — a backend file with no PG row would be // invisible to it. ON CONFLICT DO NOTHING keeps a concurrent // uploader's row (and its references) intact. - let hashes: Vec = written.iter().map(|(h, _)| h.clone()).collect(); - let sizes: Vec = written.iter().map(|(_, s)| *s).collect(); + // `written` is owned and dead after this rollback — unzip it (moving each + // 64-byte hash String out) instead of cloning every hash purely to + // reshape for `sync_blobs(&[String])` + the UNNEST bind. + // (benches/ROUND23.md §U1) + let (hashes, sizes): (Vec, Vec) = written.into_iter().unzip(); if let Err(e) = backend.sync_blobs(&hashes).await { tracing::warn!( "Ingest rollback: sync of {} chunks failed: {e}", @@ -896,8 +899,10 @@ impl DedupService { if !new_rows.is_empty() { // Durability before visibility — same invariant as the ingest // engine: no PG row may ever point at unsynced bytes. - let hashes: Vec = new_rows.iter().map(|(h, _)| h.clone()).collect(); - let sizes: Vec = new_rows.iter().map(|(_, s)| *s).collect(); + // `new_rows` is owned and dead after this block — unzip (move the + // hash Strings out) instead of cloning each one for the reshape + + // UNNEST bind. (benches/ROUND23.md §U1) + let (hashes, sizes): (Vec, Vec) = new_rows.into_iter().unzip(); self.backend.sync_blobs(&hashes).await?; sqlx::query( "INSERT INTO storage.blobs (hash, size, ref_count, orphaned_at) From ffb536e0aefbea11bc7d43b2f88ae37d6d8604cb Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 17:12:38 +0000 Subject: [PATCH 3/3] =?UTF-8?q?perf:=20round=2024=20=E2=80=94=20download?= =?UTF-8?q?=5Fzip=20per-item=20authz+metadata=20N+1=20=E2=86=92=20batch=20?= =?UTF-8?q?(validated=20authorization=20pass)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ROUND23-deferred download_zip N+1, given its own validated pass. The individually-selected files were authorized + fetched one at a time via get_file_with_perms (require + get = 2 serial round-trips/file) before any streaming — a 200-file selection was 400 serial round-trips. AFTER routes the whole multi-select through the new FileRetrievalService::get_files_by_ids_with_perms: one check_files_read_batch (the PgAclEngine resolves every file's drive in ONE query and primes the resource->drive cache) + one get_files_by_ids. 2N round-trips -> 2. Authorization is unchanged and still enforced BEFORE any ZIP entry is written: - add_file_entry_streamed writes the entry header (the filename) before it opens the authorized stream, so the pre-filter is load-bearing — a denied file must never reach it or its name leaks into the archive. AFTER a denied/missing id is absent from the authorized map and is skipped in the same input order, exactly as the old loop skipped a denied get_file_with_perms; it never reaches the entry write. The authz moved from a per-file require to one batch check EARLIER in the same function, not into or after the stream. - The stream open keeps its own per-file Read check (now a primed-cache hit) + Recents recording; check_files_read_batch is documented + gated as identical to looping require. Because the change is authorization-sensitive, the gate is the security property itself. bench_round24_zip_authz drives the real PgAclEngine over a seeded, interleaved mix of owned (granted drive) + denied (other drive) + missing ids and asserts: the batch inclusion set AND input order are identical to the per-file require loop; the included set is exactly the caller's owned files; no denied or missing id is ever included (the authz-regression tripwire); and the batch fetch returns exactly the owned files. Latency (cold, 600-item 1/3-owned selection): 559 -> 267 ms (2.10x; the realistic all-owned selection is O(1) -> a larger win). See benches/ROUND24.md. The folder selections are left as-is (root counts are small and there is no check_folders_read_batch primitive to batch through). Verified: cargo clippy --features bench --all-targets -D warnings clean, cargo fmt --all --check clean, cargo test --lib --features bench = 529 passed / 0 failed. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01DKyQ4AnYtgp1JtjzweyMeo --- Cargo.toml | 15 + benches/ROUND24.md | 150 +++++++ examples/bench_round24_zip_authz.rs | 387 ++++++++++++++++++ src/application/services/batch_operations.rs | 55 ++- .../services/file_retrieval_service.rs | 46 +++ 5 files changed, 633 insertions(+), 20 deletions(-) create mode 100644 benches/ROUND24.md create mode 100644 examples/bench_round24_zip_authz.rs diff --git a/Cargo.toml b/Cargo.toml index 71ee207f..00e65fd4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -354,6 +354,21 @@ name = "bench_micro_allocs" path = "examples/bench_micro_allocs.rs" required-features = ["bench"] +# Round-24 battery ──────────────────────────────────────────────────────────── + +# Round-24 download_zip authz+metadata N+1 → batch, VALIDATED. The per-file +# get_file_with_perms (require + get, 2 round-trips/file) becomes one +# check_files_read_batch + one get_files_by_ids. Because the change is +# authorization-sensitive, the gate is the security property: the batch check +# must make the identical per-file inclusion decision (same set AND input order) +# as the shipped-before require loop, over owned + denied + missing ids, and +# never include a denied/missing file. Drives the real PgAclEngine. Needs the +# dev Postgres up (reads DATABASE_URL from .env). +[[example]] +name = "bench_round24_zip_authz" +path = "examples/bench_round24_zip_authz.rs" +required-features = ["bench"] + # Round-23 battery ──────────────────────────────────────────────────────────── # Round-23 CPU/alloc micro-pack (no Postgres) — deterministic alloc gates for diff --git a/benches/ROUND24.md b/benches/ROUND24.md new file mode 100644 index 00000000..39af351b --- /dev/null +++ b/benches/ROUND24.md @@ -0,0 +1,150 @@ +# Round 24 — `download_zip` per-item authz+metadata N+1 → batch (validated authorization pass) + +This is the ROUND23 "not shipped" item #2, given the dedicated validated pass it +needed. Unlike the other rounds it is **authorization-sensitive**, so the gate is +not an allocation count or a latency floor — it is the **security property +itself**: the batched authorization must make the *identical* per-file inclusion +decision as the shipped-before per-file `require` loop, and must never let a +denied or missing file into the archive. + +Reproduce (needs the dev Postgres up; reads `DATABASE_URL` from `.env`): + +``` +cargo run --release --features bench --example bench_round24_zip_authz +``` + +## The change + +`BatchOperations::download_zip` streamed a client's multi-selection into a ZIP. +For the **individually-selected files** it looped, per file: + +```rust +for file_id in &file_ids { + match self.file_retrieval.get_file_with_perms(file_id, user_id).await { // require + get = 2 round-trips + Ok(file_dto) => { self.add_file_entry_streamed(&mut zip, file_id, &file_dto.name, &file_dto.mime_type, Some(user_id)).await … } + Err(_) => { /* skip + log */ } + } +} +``` + +`get_file_with_perms` is `require_file` (a `Read` authz round-trip) **plus** +`get_file` (a metadata round-trip) — so a selection of N files is **2N serial +round-trips** before a single byte is streamed. AFTER routes the whole selection +through one new service method: + +```rust +let authorized = self.file_retrieval + .get_files_by_ids_with_perms(&file_ids, user_id).await?; // 1 batch check + 1 batch get +let by_id: HashMap = authorized.into_iter() + .filter_map(|f| Uuid::parse_str(&f.id).ok().map(|u| (u, f))).collect(); +for file_id in &file_ids { // same input order + let Some(file_dto) = Uuid::parse_str(file_id).ok().and_then(|u| by_id.get(&u)) else { + info!("Skipping file {file_id} (not accessible or missing)"); continue; + }; + self.add_file_entry_streamed(&mut zip, file_id, &file_dto.name, &file_dto.mime_type, Some(user_id)).await … +} +``` + +`FileRetrievalService::get_files_by_ids_with_perms` authorizes every id in ONE +`AuthorizationEngine::check_files_read_batch` (the `PgAclEngine` override resolves +all files' drives in a single query and reuses the per-drive role cache) and +fetches only the authorized ids in ONE `get_files_by_ids`. **2N round-trips → 2.** + +### Why this is authorization-safe (the part that made it a dedicated pass) + +Three properties had to hold, all verified against the source before touching it: + +1. **Authorization still happens before any ZIP entry is written.** + `add_file_entry_streamed` writes the entry header (the **filename**) *before* + it opens the authorized stream (`write_entry_stream` then + `get_file_stream_with_perms`). So the pre-filter is load-bearing: a denied + file must never reach `add_file_entry_streamed`, or its name would leak into + the archive (and leave a dangling entry). AFTER preserves this exactly — a + denied/missing id is absent from `by_id`, so it is `continue`-skipped and + never reaches the entry write. The authz simply moved from a per-file + `require` to one batch `check` **earlier** in the same function, not into or + after the stream. + +2. **The per-file stream-open Read check + Recents recording are unchanged.** + `add_file_entry_streamed(Some(user_id))` still calls + `get_file_stream_with_perms`, which re-checks `Read` (now a primed-cache hit — + `check_files_read_batch` seeds the resource→drive cache) and records the + access in Recents. The old loop double-notified Recents (once in + `get_file_with_perms`, once in the stream open) and the throttle coalesced it + to one entry; AFTER notifies once (the stream open) — identical net effect. + +3. **The batch authorization is identical to looping `require`.** + `check_files_read_batch` is documented and gated as "semantically identical to + looping `check`", and `require(Read)` succeeds iff `check(Read)` is true (a + denied `Read` is the 404 anti-enumeration shape). The §validation gate proves + this empirically on a mix of granted / denied / missing ids. + +The **folder** selections (`get_folder_with_perms` per root, then the already-bulk +`add_folder_subtree_to_zip`) are left as-is: root counts are small and there is no +`check_folders_read_batch` primitive to batch through — see *Not shipped*. + +## The validation + +`bench_round24_zip_authz` drives the **real `PgAclEngine`** (the `fresh_engine` +shape from `bench_favorites_authz`) against a seeded fixture designed to exercise +every inclusion outcome: + +- `owned` — N files on **drive A**, which the caller holds an `editor` grant on → **must be INCLUDED** +- `denied` — N files on **drive B**, which the caller has **no** grant on → **must be DENIED** +- `missing` — N random UUIDs that don't exist → **must be MISSING** + +interleaved `owned, denied, missing, owned, …` so the **order** test is real. The +gate asserts, and `exit(1)`s on any failure: + +- `before_included` (the per-file `require` filter, in input order) **==** + `after_included` (the batch `check_files_read_batch` filter, in input order) — + identical **set and order**; +- the included set is **exactly** the caller's `owned` files; +- **no** `denied` (other-drive) file is included — the authz-regression tripwire; +- **no** `missing` id is included; +- the batch `get_files_by_ids` of the authorized ids returns **exactly** the + `owned` files. + +Latency (cold engine, empty caches — the first-download shape), `BENCH_FILES=200` +(600-item interleaved selection, ⅓ owned / ⅓ denied / ⅓ missing): + +| arm | wall (600 items) | per file | +|---|---|---| +| per-file `require` loop | 559.47 ms | 932.45 µs | +| batch `check_files_read_batch` | 266.58 ms | 444.30 µs | + +**2.10×** — and this is the *conservative* case: with ⅓ of the ids on a drive +the caller has no role on, `check_files_read_batch` still falls back to a per-file +`check_inner` for each un-readable-drive file. The realistic "download my own N +files" selection is **all** on drives the caller has a role on, where the batch +is genuinely O(1) (one drive-resolve query + cached role checks) against the +loop's 2N round-trips — a far larger win. + +## Not shipped + +- **Folder selections** (`download_zip`'s folder loop): `get_folder_with_perms` + per selected root. Root counts are typically 1–3, and there is no + `check_folders_read_batch` batch-authz primitive (only files have one), so + batching would still loop `check` per root — no round-trip win. Left as-is. +- **Dropping the stream-open re-check**: since the batch pre-check already + authorized (and primed the cache), `add_file_entry_streamed`'s + `get_file_stream_with_perms` re-check is now redundant (a cache hit). Replacing + it with the no-perms `get_file_stream` would save the cache lookups but would + also drop the Recents recording and the second authz barrier — not worth the + behavior change; kept as belt-and-suspenders. + +## Environment / methodology + +- Real `PgAclEngine` + `FileBlobReadRepository` against a local **PostgreSQL 16** + (schema from `migrations/`). The bench seeds its own two-drive fixture + (`bench_zipauthz_*` markers) and tears it down around the run. +- Built with `RUSTFLAGS="-C target-cpu=x86-64-v3"` (this session's host + intermittently `SIGILL`ed rustc under the repo's default `-C target-cpu=native` + AVX-512 after a host migration — see benches/ROUND23.md). Local build-flag + override only; the checked-in `.cargo/config.toml` is unchanged. +- The gate is the security equivalence (set + order + denied/missing exclusion), + not a perf threshold; the latency table is supporting evidence for the + round-trip collapse. +- Verified beyond the bench: `cargo clippy --features bench --all-targets + -D warnings` clean, `cargo fmt --all --check` clean, `cargo test --lib + --features bench` = 529 passed / 0 failed. diff --git a/examples/bench_round24_zip_authz.rs b/examples/bench_round24_zip_authz.rs new file mode 100644 index 00000000..8857ce56 --- /dev/null +++ b/examples/bench_round24_zip_authz.rs @@ -0,0 +1,387 @@ +//! Round-24 — `download_zip` per-item authz+metadata N+1 → batch, VALIDATED. +//! +//! `BatchOperations::download_zip` authorized + fetched each selected file with +//! a per-file `get_file_with_perms` (= `require_file` authz + `get_file`) — 2 +//! serial round-trips per file, before any streaming. AFTER routes the whole +//! multi-select through `FileRetrievalService::get_files_by_ids_with_perms`, +//! which authorizes every id in ONE `check_files_read_batch` and fetches the +//! authorized ids in ONE `get_files_by_ids` (2 round-trips total). The +//! subsequent `add_file_entry_streamed` keeps its own per-file stream-open Read +//! check + Recents recording (now a primed-cache hit), so authorization still +//! happens BEFORE any ZIP entry is written — a denied file never leaks its name. +//! +//! Because this change is authorization-sensitive, the gate is the security +//! property itself: the batch `check_files_read_batch` must make the EXACT same +//! per-file inclusion decision as the shipped-before per-file `require` loop — +//! same **set** AND same **input order** — over a mix of +//! • files on a drive the caller is granted `editor` on (INCLUDED) +//! • files on a drive the caller has NO grant on (DENIED) +//! • ids that don't exist at all (MISSING) +//! and the batch fetch must return exactly the authorized, existing files. +//! Any divergence `std::process::exit(1)`s. +//! +//! Drives the REAL `PgAclEngine` + `FileBlobReadRepository` (the fresh_engine +//! shape from bench_favorites_authz). +//! +//! Run (needs Postgres up; reads DATABASE_URL from .env): +//! cargo run --release --features bench --example bench_round24_zip_authz +//! Tunables (env): BENCH_FILES (200), BENCH_POOL (20). + +use std::collections::HashSet; +use std::env; +use std::sync::Arc; +use std::time::{Duration, Instant}; + +use oxicloud::application::ports::authorization_ports::AuthorizationEngine; +use oxicloud::domain::services::authorization::{Permission, Resource, Subject}; +use oxicloud::infrastructure::repositories::pg::{ + FileBlobReadRepository, FolderDbRepository, SubjectGroupPgRepository, +}; +use oxicloud::infrastructure::services::dedup_service::DedupService; +use oxicloud::infrastructure::services::local_blob_backend::LocalBlobBackend; +use oxicloud::infrastructure::services::pg_acl_engine::PgAclEngine; +use sqlx::PgPool; +use sqlx::postgres::PgPoolOptions; +use uuid::Uuid; + +fn env_or(key: &str, default: T) -> T { + env::var(key) + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(default) +} + +struct Seeded { + caller: Uuid, + other: Uuid, + drive_a: Uuid, + drive_b: Uuid, + root_a: Uuid, + root_b: Uuid, + blob_hash: String, + /// The caller's accessible files (drive A) — the expected INCLUDED set. + owned: Vec, + /// Files on drive B (no grant to caller) — expected DENIED. + denied: Vec, + /// Non-existent ids — expected MISSING. + missing: Vec, + /// The full selection, interleaved owned/denied/missing (order matters). + selection: Vec, +} + +async fn seed(pool: &PgPool, n_files: usize) -> Seeded { + let mut tx = pool.begin().await.expect("begin"); + let caller: Uuid = sqlx::query_scalar( + "INSERT INTO auth.users (username, email, role) + VALUES ('bench_zipauthz_a', 'bench_zipauthz_a@bench.invalid', 'user') RETURNING id", + ) + .fetch_one(&mut *tx) + .await + .expect("seed caller"); + let other: Uuid = sqlx::query_scalar( + "INSERT INTO auth.users (username, email, role) + VALUES ('bench_zipauthz_b', 'bench_zipauthz_b@bench.invalid', 'user') RETURNING id", + ) + .fetch_one(&mut *tx) + .await + .expect("seed other"); + + let blob_hash = "benchzipauthz00000000000000000000000000000000000000000000000b24".to_string(); + sqlx::query("INSERT INTO storage.blobs (hash, size, ref_count) VALUES ($1, 1, 1)") + .bind(&blob_hash) + .execute(&mut *tx) + .await + .expect("seed blob"); + + // Two shared drives; `caller` is granted editor on A only, `other` on B. + async fn drive_with_grant( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + label: &str, + grantee: Uuid, + ) -> (Uuid, Uuid) { + let drive: Uuid = + sqlx::query_scalar("INSERT INTO storage.drives (kind) VALUES ('shared') RETURNING id") + .fetch_one(&mut **tx) + .await + .expect("seed drive"); + let root: Uuid = sqlx::query_scalar( + "INSERT INTO storage.folders (name, path, lpath, drive_id) + VALUES ($1, $2, 'x', $3) RETURNING id", + ) + .bind(format!("Bench {label}")) + .bind(format!("/Bench {label}")) + .bind(drive) + .fetch_one(&mut **tx) + .await + .expect("seed folder"); + sqlx::query("UPDATE storage.drives SET root_folder_id = $1 WHERE id = $2") + .bind(root) + .bind(drive) + .execute(&mut **tx) + .await + .expect("stamp root"); + sqlx::query( + "INSERT INTO storage.role_grants + (subject_type, subject_id, resource_type, resource_id, role, granted_by) + VALUES ('user', $1, 'drive', $2, 'editor'::storage.grant_role, $1)", + ) + .bind(grantee) + .bind(drive) + .execute(&mut **tx) + .await + .expect("seed grant"); + (drive, root) + } + + let (drive_a, root_a) = drive_with_grant(&mut tx, "A", caller).await; + let (drive_b, root_b) = drive_with_grant(&mut tx, "B", other).await; + + let mut owned = Vec::with_capacity(n_files); + let mut denied = Vec::with_capacity(n_files); + for i in 0..n_files { + for (drive, root, sink) in [ + (drive_a, root_a, &mut owned), + (drive_b, root_b, &mut denied), + ] { + let id: Uuid = sqlx::query_scalar( + "INSERT INTO storage.files (name, folder_id, blob_hash, size, mime_type, drive_id) + VALUES ($1, $2, $3, 1, 'text/plain', $4) RETURNING id", + ) + .bind(format!("bench-{i:04}.txt")) + .bind(root) + .bind(&blob_hash) + .bind(drive) + .fetch_one(&mut *tx) + .await + .expect("seed file"); + sink.push(id); + } + } + tx.commit().await.expect("commit"); + + let missing: Vec = (0..n_files).map(|_| Uuid::new_v4()).collect(); + + // Interleave owned / denied / missing so the order test is meaningful. + let mut selection = Vec::with_capacity(n_files * 3); + for i in 0..n_files { + selection.push(owned[i]); + selection.push(denied[i]); + selection.push(missing[i]); + } + + Seeded { + caller, + other, + drive_a, + drive_b, + root_a, + root_b, + blob_hash, + owned, + denied, + missing, + selection, + } +} + +async fn cleanup(pool: &PgPool, s: &Seeded) { + for d in [s.drive_a, s.drive_b] { + let _ = sqlx::query("DELETE FROM storage.role_grants WHERE resource_id = $1") + .bind(d) + .execute(pool) + .await; + let _ = sqlx::query("DELETE FROM storage.files WHERE drive_id = $1") + .bind(d) + .execute(pool) + .await; + let _ = sqlx::query("DELETE FROM storage.drives WHERE id = $1") + .bind(d) + .execute(pool) + .await; + } + for f in [s.root_a, s.root_b] { + let _ = sqlx::query("DELETE FROM storage.folders WHERE id = $1") + .bind(f) + .execute(pool) + .await; + } + let _ = sqlx::query("DELETE FROM storage.blobs WHERE hash = $1") + .bind(&s.blob_hash) + .execute(pool) + .await; + let _ = sqlx::query("DELETE FROM auth.users WHERE id IN ($1, $2)") + .bind(s.caller) + .bind(s.other) + .execute(pool) + .await; +} + +fn fresh_engine(pool: &Arc) -> (Arc, Arc) { + let folder_repo = Arc::new(FolderDbRepository::new(pool.clone())); + let backend = Arc::new(LocalBlobBackend::new(std::path::Path::new( + "/tmp/bench-zipauthz-blobs", + ))); + let dedup = Arc::new(DedupService::new(backend, pool.clone(), pool.clone())); + let file_repo = Arc::new(FileBlobReadRepository::new( + pool.clone(), + dedup, + folder_repo.clone(), + )); + let group_repo = Arc::new(SubjectGroupPgRepository::new(pool.clone())); + let engine = Arc::new(PgAclEngine::new( + pool.clone(), + folder_repo, + file_repo.clone(), + group_repo, + )); + (engine, file_repo) +} + +/// BEFORE, verbatim: the per-file `require` filter, preserving input order. +async fn before_included(engine: &PgAclEngine, user: Uuid, sel: &[Uuid]) -> Vec { + let mut out = Vec::new(); + for id in sel { + if engine + .require(Subject::User(user), Permission::Read, Resource::File(*id)) + .await + .is_ok() + { + out.push(*id); + } + } + out +} + +/// AFTER: one batch check, then re-associate in input order (the download_zip +/// re-association). +async fn after_included(engine: &PgAclEngine, user: Uuid, sel: &[Uuid]) -> Vec { + let allowed: HashSet = engine + .check_files_read_batch(Subject::User(user), sel) + .await + .expect("batch check"); + sel.iter() + .copied() + .filter(|id| allowed.contains(id)) + .collect() +} + +#[tokio::main(flavor = "multi_thread")] +async fn main() { + dotenvy::dotenv().ok(); + let url = env::var("DATABASE_URL") + .or_else(|_| env::var("OXICLOUD_DB_CONNECTION_STRING")) + .expect("set DATABASE_URL — the dev Postgres URL"); + let n_files: usize = env_or("BENCH_FILES", 200); + let pool_size: u32 = env_or("BENCH_POOL", 20); + + let pool = Arc::new( + PgPoolOptions::new() + .max_connections(pool_size) + .min_connections(pool_size) + .acquire_timeout(Duration::from_secs(10)) + .connect(&url) + .await + .expect("connect Postgres"), + ); + + // Clear any prior fixtures, then seed. + let _ = sqlx::query( + "DELETE FROM auth.users WHERE email IN ('bench_zipauthz_a@bench.invalid','bench_zipauthz_b@bench.invalid')", + ) + .execute(pool.as_ref()) + .await; + let seeded = seed(&pool, n_files).await; + + // ── Equivalence gate (fresh engines so neither arm rides the other's cache) ── + let (eng_before, _) = fresh_engine(&pool); + let (eng_after, file_repo) = fresh_engine(&pool); + let before = before_included(&eng_before, seeded.caller, &seeded.selection).await; + let after = after_included(&eng_after, seeded.caller, &seeded.selection).await; + + let owned_set: HashSet = seeded.owned.iter().copied().collect(); + let denied_set: HashSet = seeded.denied.iter().copied().collect(); + let missing_set: HashSet = seeded.missing.iter().copied().collect(); + + let mut fail = false; + if before != after { + eprintln!("GATE FAIL: batch inclusion set/order != per-file require loop"); + fail = true; + } + // The included set must be EXACTLY the caller's owned files, in input order. + let expected: Vec = seeded + .selection + .iter() + .copied() + .filter(|id| owned_set.contains(id)) + .collect(); + if after != expected { + eprintln!("GATE FAIL: included set is not exactly the caller's owned files (in order)"); + fail = true; + } + if after.iter().any(|id| denied_set.contains(id)) { + eprintln!("GATE FAIL: a DENIED (other-drive) file was included — authz regression!"); + fail = true; + } + if after.iter().any(|id| missing_set.contains(id)) { + eprintln!("GATE FAIL: a MISSING id was included"); + fail = true; + } + // The batch fetch of the authorized ids must return exactly those files. + let allowed_ids: Vec = after.iter().map(Uuid::to_string).collect(); + let fetched = file_repo + .get_files_by_ids(&allowed_ids) + .await + .expect("batch fetch"); + let fetched_ids: HashSet = fetched + .iter() + .filter_map(|f| Uuid::parse_str(f.id()).ok()) + .collect(); + if fetched_ids != owned_set { + eprintln!("GATE FAIL: batch fetch of authorized ids != owned files"); + fail = true; + } + if fail { + cleanup(&pool, &seeded).await; + std::process::exit(1); + } + + println!("\n#################################################################"); + println!("# download_zip authz+metadata: per-file require loop vs batch"); + println!( + "# selection = {n} owned + {n} denied + {n} missing (interleaved)", + n = n_files + ); + println!("# gate OK: identical inclusion set+order; denied+missing excluded;"); + println!( + "# batch fetch returns exactly the {} owned files.", + seeded.owned.len() + ); + println!("#################################################################\n"); + println!("| {:<26} | {:>10} | {:>12} |", "arm", "wall ms", "µs/file"); + + // Latency: cold engine each run (empty caches — the first-download shape). + let total = seeded.selection.len(); + for (label, batch) in [ + ("per-file require loop", false), + ("batch check_files_read", true), + ] { + let (engine, _) = fresh_engine(&pool); + let t = Instant::now(); + let got = if batch { + after_included(&engine, seeded.caller, &seeded.selection).await + } else { + before_included(&engine, seeded.caller, &seeded.selection).await + }; + let el = t.elapsed(); + assert_eq!(got.len(), seeded.owned.len(), "arm {label} inclusion count"); + println!( + "| {:<26} | {:>10.2} | {:>12.2} |", + label, + el.as_secs_f64() * 1e3, + el.as_secs_f64() * 1e6 / total as f64 + ); + } + + cleanup(&pool, &seeded).await; + println!("\nAll Round-24 authz-equivalence gates passed."); +} diff --git a/src/application/services/batch_operations.rs b/src/application/services/batch_operations.rs index bd75d2c5..8f249b29 100644 --- a/src/application/services/batch_operations.rs +++ b/src/application/services/batch_operations.rs @@ -726,31 +726,46 @@ impl BatchOperationService { let mut items_added: usize = 0; // ── Add individual files at the root of the ZIP ────────────────── + // Authorize + fetch metadata for the whole multi-select in 2 round-trips + // (one batch Read check + one batch get) instead of the per-file + // `get_file_with_perms` N+1 (2 round-trips/file). The batch check also + // primes the resource→drive cache, so `add_file_entry_streamed`'s + // per-file stream-open re-check lands on the cache. A denied / missing / + // unparseable id is absent from the map → skipped in the same input + // order, exactly as the old per-file loop skipped it. Authorization is + // UNCHANGED — still enforced (pre-check here + the stream open's own + // Read check + Recents recording) before any ZIP entry is written, so a + // denied file never leaks its name into the archive (benches/ROUND24.md). + let authorized = self + .file_retrieval + .get_files_by_ids_with_perms(&file_ids, user_id) + .await + .map_err(BatchOperationError::Domain)?; + let by_id: HashMap = authorized + .into_iter() + .filter_map(|f| Uuid::parse_str(&f.id).ok().map(|u| (u, f))) + .collect(); for file_id in &file_ids { + let file_dto = match Uuid::parse_str(file_id).ok().and_then(|u| by_id.get(&u)) { + Some(f) => f, + None => { + info!("Skipping file {} (not accessible or missing)", file_id); + continue; + } + }; match self - .file_retrieval - .get_file_with_perms(file_id, user_id) + .add_file_entry_streamed( + &mut zip, + file_id, + &file_dto.name, + &file_dto.mime_type, + Some(user_id), + ) .await { - Ok(file_dto) => { - match self - .add_file_entry_streamed( - &mut zip, - file_id, - &file_dto.name, - &file_dto.mime_type, - Some(user_id), - ) - .await - { - Ok(_) => items_added += 1, - Err(e) => { - info!("Could not add file {} to ZIP: {}", file_dto.name, e); - } - } - } + Ok(_) => items_added += 1, Err(e) => { - info!("Could not get file metadata {}: {}", file_id, e); + info!("Could not add file {} to ZIP: {}", file_dto.name, e); } } } diff --git a/src/application/services/file_retrieval_service.rs b/src/application/services/file_retrieval_service.rs index 882ecb46..b0262e28 100644 --- a/src/application/services/file_retrieval_service.rs +++ b/src/application/services/file_retrieval_service.rs @@ -287,6 +287,52 @@ impl FileRetrievalService { Ok(files.into_iter().map(FileDto::from).collect()) } + /// Batched, authorized multi-get for the ZIP-download multi-select — the + /// batch form of [`FileRetrievalUseCase::get_file_with_perms`] over an + /// explicit id list. + /// + /// Authorizes `Read` on every id in ONE `check_files_read_batch` + /// round-trip (which resolves all drives in a single query AND primes the + /// resource→drive cache, so the per-file re-check the subsequent stream + /// open performs becomes a cache hit), then fetches only the authorized ids + /// in ONE `get_files_by_ids` query. Replaces `download_zip`'s per-file + /// `require_file` + `get_file` loop — 2 round-trips/file → 2 total. + /// + /// Returns the authorized, existing files; a denied / missing / unparseable + /// id is simply **absent** from the result (the caller re-associates by id + /// and skips the rest, exactly as the per-file loop skipped a denied / + /// missing `get_file_with_perms`). Read-authorization is identical to the + /// per-file path (`check_files_read_batch` is documented and gated as + /// semantically identical to looping `require`). Recents recording is left + /// to the subsequent per-file stream open (`get_file_stream_with_perms`), + /// which records it (throttle-coalesced) — same net effect as the old + /// loop's `notify_file_accessed` + stream double-notify. Fail-closed if no + /// engine was injected, mirroring [`Self::require_file`]. + pub async fn get_files_by_ids_with_perms( + &self, + ids: &[String], + caller_id: Uuid, + ) -> Result, DomainError> { + let authz = self.authz.as_ref().ok_or_else(|| { + DomainError::internal_error("FileRetrieval", "Authorization engine unavailable") + })?; + // Unparseable ids can't be authorized (the per-file path 404s on them), + // so drop them here — they stay absent from the authorized set. + let uuids: Vec = ids.iter().filter_map(|s| Uuid::parse_str(s).ok()).collect(); + if uuids.is_empty() { + return Ok(Vec::new()); + } + let allowed = authz + .check_files_read_batch(Subject::User(caller_id), &uuids) + .await?; + if allowed.is_empty() { + return Ok(Vec::new()); + } + let allowed_ids: Vec = allowed.iter().map(Uuid::to_string).collect(); + let files = self.file_read.get_files_by_ids(&allowed_ids).await?; + Ok(files.into_iter().map(FileDto::from).collect()) + } + /// Range read for HTTP Range Requests, cache-aware. /// /// Media players and PDF viewers fetch these files *exclusively* through