diff --git a/Cargo.toml b/Cargo.toml index c9f7dbd0..44caf0f3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -358,6 +358,17 @@ name = "bench_micro_allocs" path = "examples/bench_micro_allocs.rs" required-features = ["bench"] +# Round-29 battery ──────────────────────────────────────────────────────────── + +# Round-29 CPU/alloc micro-pack (no Postgres) — NC REPORT per-row href String(s) +# → reused buffer via nc_href_into with a once-encoded user (A); cache-serve fast +# path eager get_or_load args (etag/key/id) built before the borrow-probe hit (B); +# read_full single-frame BytesMut concat → zero-copy passthrough (C). +[[example]] +name = "bench_round29_micro" +path = "examples/bench_round29_micro.rs" +required-features = ["bench"] + # Round-27 battery ──────────────────────────────────────────────────────────── # Round-27 CPU/alloc micro-pack (no Postgres) — NC PROPFIND per-row oc:id String diff --git a/benches/ROUND29.md b/benches/ROUND29.md new file mode 100644 index 00000000..af211222 --- /dev/null +++ b/benches/ROUND29.md @@ -0,0 +1,244 @@ +# Round 29 — read-path cache-serve allocs, NC REPORT href buffer, auth per-request allocs, DB over-fetch + +Seven behaviour-preserving cuts, each behind a counting-allocator BEFORE/AFTER gate +that `exit(1)`s (`GATE FAIL … rollback`) unless AFTER allocates strictly fewer than +BEFORE. Sections span the four hot paths a deep re-audit surfaced that the prior 28 +rounds had not reached: the content-cache serve fast path (video scrubbing), the +NextCloud REPORT emit loops, the NextCloud Basic-Auth request path, and two +Postgres over-fetch sites. + +Reproduce: + +```bash +RUSTFLAGS="-C target-cpu=x86-64-v3" \ + cargo run --release --features bench --example bench_round29_micro +``` + +| § | site | allocs/op BEFORE→AFTER | wall | +|----|------|-----------------------:|-----:| +| A | NC REPORT href reused buffer | 3500 → 2003 /500-row page | 1.59× | +| B | cache-serve borrow-probe (video scrub) | 6 → 0 /cache hit | 5.85× | +| C | `read_full` single-frame zero-copy | 1 → 0 | 194× | +| D | login-lockout single-alloc key | 3 → 1 | 2.10× | +| E | NC composite-username parse borrow | 1 → 0 | 2.86× | +| F | contact-group `vcard` over-fetch | 200 → 0 /200-row page | decode-shape | +| G | admin-count `COUNT(*)` vs hydrate | 25 → 0 /poll | decode-shape | + +--- + +## [B] Content-cache serve fast path: eager owned args built before the borrow-probe (HIGHEST — the hottest read path) + +`file_retrieval_service::optimized_inner` (Tier 1) and `get_file_range_preloaded` +built the owned `get_or_load` arguments — `format!("\"{}\"", hash)` (the quoted +etag), `hash.to_string()` (the cache key), `id.to_string()` — **before** the cache +was probed. But `FileContentCache::get_or_load`'s first line is a lock-free +`self.get(&cache_key)` that returns on a hit and never touches `etag` / `ct` / the +load closure. So every cache **hit** — the steady state of a repeat download and of +a *range-seek storm* (video scrubbing hits `get_file_range_preloaded` on every +seek) — allocated ~3–6 Strings and immediately dropped them. The returned etag/ct +are discarded by both callers (`let (bytes, ..)`), and the response etag is built +independently from `file_dto.etag`, so the eager etag was dead on the miss path too. + +AFTER probes `cache.get(&hash)` (a borrow, zero owned allocs) first and slices on a +hit; only a miss builds the owned args and calls the new `load_and_cache`. +`get_or_load` is split into `get` + `load_and_cache` (it now composes them), so the +miss path is **not** re-probed — the hit/miss stat counters stay byte-identical to a +single `get_or_load` call. Also folds in the removal of the unconditional +`content_hash.clone()` + `name.clone()` that ran for every request including the +≥10 MB streaming tier that used neither. + +| arm | ns/op | allocs/op | +|--------|------:|----------:| +| BEFORE | 241.1 | 6.00 | +| AFTER | 41.2 | 0.00 | + +**6 → 0 allocs per cache hit, 5.85× wall.** On a 200-seek video scrub this removes +~1200 throwaway allocations. Equivalence: same cached `Bytes` returned; the split +preserves the exact single-`get` stat accounting. + +## [A] NextCloud REPORT emit loops: per-row href String → one reused buffer + once-encoded user + +The two REPORT handlers (`report_handler`: favorites `filter-files` + `search`) +each emit a file loop and a folder loop that built `` per row with +`nc_href(url_user, subpath)` — a fresh `String` per file row — and +`format!("{}/", nc_href(...))` — **two** Strings per folder row — while re-encoding +the constant `url_user` on every row. The hotter PROPFIND child loop was already +hoisted to a reused buffer + once-encoded prefix (ROUND19/27); the REPORT loops were +the last per-row href allocation on the NC emit surface (the ROUND20/27/28 deferred +item). AFTER adds `nc_href_into` / `nc_collection_href_into` (the 0-alloc, +write-into-a-buffer form; `nc_href`/`nc_collection_href` now delegate to them, no +duplication) and computes into one `href_buf` reused across both loops with the +`encoded_user` computed once per page. + +| arm | ns/op | allocs/op | +|--------|----------:|----------:| +| BEFORE | 115 849.2 | 3500.00 | +| AFTER | 72 835.0 | 2003.00 | + +**1497 fewer allocs on a 500-row page, 1.59× wall.** The 2003 residual is the +per-segment `urlencoding::encode` (4 path segments/row) that AFTER keeps to stay +byte-identical; the win is the removed per-row href `String`, the folder `format!`, +and the per-row user encode. Equivalence: AFTER href bytes match BEFORE +(file + folder) across a matrix of paths. + +## [C] `read_full`: single-frame blob no longer double-copied + +`read_full` reassembled the blob stream with `BytesMut::with_capacity(cap)` + +`extend_from_slice` per frame. The local backend yields owned contiguous `Bytes` +frames, and a sub-`CACHE_THRESHOLD` blob arrives as exactly **one** frame — yet the +old code copied that whole payload a second time into a fresh buffer (a full-payload +memcpy + a `BytesMut` alloc) for every small cacheable download and every +uncacheable small read. AFTER returns the sole frame directly; only a multi-frame +read pays the pre-sized concat (byte-identical). + +| arm | ns/op | allocs/op | +|--------|------:|----------:| +| BEFORE | 3325.9 | 1.00 | +| AFTER | 17.2 | 0.00 | + +**1 → 0 allocs and one 200 KB memcpy removed (194× wall on the isolated copy).** +Equivalence: identical `Bytes` out; multi-frame path unchanged. + +## [D] NextCloud login-lockout key: `to_lowercase()` + `format!` → one ASCII buffer + +`LoginLockoutService::key` built the composite `(account, IP)` cache key with +`format!("{}|{}", username.to_lowercase(), client_ip)` — two heap allocations — on +**every** NC request (the check on the way in; a hit on the happy path is a lockout +miss). App passwords authenticate with an already-lowercase ASCII username in ~all +traffic, so AFTER renders the lowercased key into one pre-sized buffer for the ASCII +case and keeps `str::to_lowercase` only on the rare non-ASCII branch (exact Unicode, +e.g. final-sigma, semantics). + +| arm | ns/op | allocs/op | +|--------|------:|----------:| +| BEFORE | 95.7 | 3.00 | +| AFTER | 45.7 | 1.00 | + +**3 → 1 alloc, 2.10× wall.** Byte-identical key verified across {ASCII lower, +mixed-case, composite `~` marker, IPv4, IPv6, non-ASCII, `unknown`}. The lockout +decision (same key bytes, threshold, TTL) is unchanged; failed verifications still +bypass the cache and pay full Argon2. + +## [E] NextCloud composite-username parse: owned clone → borrow + +The `{username}~{drive_marker}` split allocated the prefix per request — +`raw_username.clone()` on the common no-marker path (a full duplicate), +`u.to_string()` + `m.to_string()` on the marker path — even though `username` is +only ever passed by reference and `raw_username` outlives every use before it moves +into `NcSession`. AFTER borrows `&str` slices out of the already-owned +`raw_username`. + +| arm | ns/op | allocs/op | +|--------|------:|----------:| +| BEFORE | 22.5 | 1.00 | +| AFTER | 7.9 | 0.00 | + +**1 → 0 allocs on the common DAV path, 2.86× wall.** Stacks with §D on the same +per-request surface. Byte-identical inputs reach every downstream call. + +## [F] Contact-group listing: stop fetching the multi-KB `vcard` only to drop it + +`contact_group_pg_repository::get_contacts_in_group` SELECTed `c.vcard` — the full +serialized vCard TEXT with an embedded base64 `PHOTO`, the largest column — and +decoded it into a `String` per contact, but its sole live caller +(`list_contacts_in_group`) maps every row to `ContactDto`, which has **no vcard +field**, so it was fetched, shipped, decoded, and dropped. This is the ROUND25 §Q2 +`row_to_contact_lite` treatment applied to the **live** group method this time (Q2 +shipped it to `get_contacts_by_group`, which has zero call sites). AFTER omits the +column and passes `String::new()`. + +| arm | ns/op | allocs/op | +|--------|---------:|----------:| +| BEFORE | 70 121.1 | 200.00 | +| AFTER | 1.4 | 0.00 | + +The micro isolates the discarded-`String` decode (200 rows × 8 KiB): **200 → 0 +per-row allocs**. The new SQL was run against a live schema (all columns resolve, +join valid, empty and populated results correct); `ContactDto` output is +byte-identical. Same unit economics ROUND25 §Q2 *measured* (6.4× wall on 1000 × +8 KiB vCards). Bandwidth win scales with the embedded-photo size. + +## [G] admin-user count: hydrate every full row → scalar `COUNT(*)` + +`count_admin_users` (the system-status / initialization endpoint, polled at +bootstrap / login-page render) called `list_users_by_role("admin").len()`, fetching +every admin's full 21-column row — including the up-to-512 KiB avatar `image` data +URI and the `ui_preferences` JSONB (decoded into a discarded `serde_json::Value` +DOM) — only to take the length. AFTER adds `count_users_by_role` → +`SELECT COUNT(*) … WHERE role::text = $1` through the existing domain-trait / port +delegation pattern. + +| arm | ns/op | allocs/op | +|--------|---------:|----------:| +| BEFORE | 12 634.9 | 25.00 | +| AFTER | 0.7 | 0.00 | + +The micro isolates the hydrate-N-rows-then-`len` cost (3 admins × a 64 KiB avatar + +JSONB DOM): **25 → 0 allocs**. Validated on a live DB with 3 seeded admins carrying +200 KiB avatars: the `COUNT(*)` returns the correct `3` while the wire payload drops +from **600 000 bytes** (the three avatars) + JSONB to **8 bytes**, and the app +hydrates zero `User` structs. Win scales with admin count, avatar size, and +PG-connection distance. + +--- + +## Not shipped — carried forward + +Concrete, still-valuable items surfaced by the same re-audit, deferred here because +they need a fixture this round can't drive, a structural change wider than an +allocation cut, or a live-DB validation harness: + +- **Delta `store_loose_chunks` check-then-write (highest-value dedup item).** The + delta upload path writes every received chunk to the backend unconditionally, then + registers with `ON CONFLICT DO NOTHING` — unlike the main `settle_batch` ingest, + which runs one `WHERE hash = ANY($1)` existence probe per batch and writes only + absent chunks (the discipline the S3 backend's dropped-HEAD comment already + assumes). Bringing the delta path to parity eliminates redundant disk writes / + object-store PUTs for content the server already has (multi-tenant overlap, + abandoned-upload orphan re-sends). Deferred: near-zero on a single-tenant local + server (the highest win is on S3/Azure), it restructures the ingest, and its gate + is a backend-write-count harness (not the allocator), so it wants its own pass. The + frontend already negotiates a Dropbox-style batched have/need exchange, so the + client does **not** re-upload content the server has — this is purely the + server-side write. +- **`ingest_chunks_from_stream` end-of-stream reshape move.** The final chunk + registration clones every newly-written 64-byte hash to reshape for `sync_blobs` + + the UNNEST bind (`~4000 String allocs on a 1 GB upload`); the sibling sites were + converted to `into_iter().unzip()` moves in ROUND23/25 but this one wasn't. + Deferred: `st.written` must be restored on the two fallible error paths before + `guard.rollback()` (which itself `mem::take`s it), so the move needs a + `rollback_with_written` variant — error-path surgery on the ingest correctness path + for a once-per-upload (not per-frame) alloc cut. +- **NC `parse_basic_auth` credential borrow.** The shared helper returns + `(String, String)` via two `to_string()`s; the native Basic path already hands + `credentials.split_once(':')` `&str` borrows to `verify_basic_auth`. Bringing NC to + parity removes 2 allocs/request but touches a unit-tested shared helper and wants a + `decode_basic_credentials` extraction to avoid a third copy of the base64 logic. +- **DB `create_folder` 2 round-trips → 1 `INSERT … SELECT … RETURNING`** (the drive_id + is a pure function of the parent; `move_folder` already folds this). Needs the + `RowNotFound → not_found` branch and a live-DB gate on the dup-name / missing-parent + outcomes. +- **DB `list_users` / `search_users` lite SELECT** (drop `password_hash` + + `ui_preferences`, neither in `UserDto`) and **contacts `(address_book_id, full_name, + first_name, last_name)` composite index** for the paginated `ORDER BY`. +- **File-metadata short-TTL cache** so a range-seek storm stops re-`SELECT`ing the + whole file row after the first seek (ROUND7 removed the per-seek authz; the metadata + read remains). A genuinely new cache + write-invalidation wiring — its own validated + pass. +- **S3 read zero-copy forward** and the encrypted `PLAINTEXT_EMIT_SIZE` bump — the + ROUND25–28 carried-forward items needing MinIO / real-backend fixtures. + +## Environment / methodology + +- Counting global allocator (`examples/bench_round29_micro.rs`), no Postgres for the + gate. Each section is BEFORE (replica of the shipped-before shape) vs AFTER (replica + of the shipped-after shape, which the source now matches) with a value-equivalence + assertion and a `GATE FAIL … rollback` `exit(1)` if AFTER doesn't allocate fewer + than BEFORE. §F and §G additionally validated against a live PostgreSQL 16 with the + full migration set applied and a seeded fixture (query validity, result equivalence, + wire-byte delta). +- Built with `RUSTFLAGS="-C target-cpu=x86-64-v3"` (the checked-in + `.cargo/config.toml` pins `target-cpu=native`, which `SIGILL`s on this host). +- Verified beyond the bench: `cargo fmt --all --check` clean, + `cargo clippy --all-features --all-targets -- -D warnings` clean, + `cargo test --lib` green. diff --git a/examples/bench_round29_micro.rs b/examples/bench_round29_micro.rs new file mode 100644 index 00000000..5dcc665b --- /dev/null +++ b/examples/bench_round29_micro.rs @@ -0,0 +1,497 @@ +//! Round-29 CPU/alloc micro-pack (no Postgres). +//! +//! Same rule as ROUND2–28: BEFORE (replica of the shipped-before shape) vs AFTER +//! (replica of the shipped-after shape, which the source is then made to match), +//! with a value-equivalence gate and a `GATE FAIL … rollback` `exit(1)` if the +//! AFTER arm fails to beat BEFORE on allocs/op. +//! +//! [A] NextCloud REPORT emit loops (`report_handler`) still build each row's +//! `` with `nc_href(url_user, subpath)` — a fresh `String` per file +//! row, and `format!("{}/", nc_href(...))` (TWO Strings) per folder row — +//! re-encoding the constant `url_user` on every row. The hotter PROPFIND +//! child loop was already hoisted to a reused buffer + once-encoded prefix +//! (webdav_handler.rs child loop). AFTER mirrors that: `nc_href_into` +//! writes into one reused buffer with a precomputed `encoded_user`. +//! +//! [B] The cache-serve fast path (`file_retrieval_service::optimized_inner` +//! Tier 1 and `get_file_range_preloaded` — the video-scrub hot path) builds +//! the owned `get_or_load` args (`format!("\"{}\"", hash)` etag, the +//! `hash.to_string()` key, `id.to_string()`) BEFORE the cache is probed. +//! `get_or_load`'s first line is a lock-free `self.get(&key)` that returns +//! on a hit and never touches any of them — so a cache HIT throws all of +//! them away. AFTER probes `cache.get(&hash)` (a borrow) first and builds +//! the owned args only on a miss. +//! +//! [C] `file_retrieval_service::read_full` reassembles the blob stream with +//! `BytesMut::with_capacity(cap)` + `extend_from_slice` per frame. The local +//! backend yields owned contiguous `Bytes` frames; for a file that fits in +//! one frame (≤256 KB) this copies the whole payload a SECOND time into a +//! fresh buffer. AFTER returns the sole frame directly (zero-copy) and only +//! falls back to the pre-sized concat when there is more than one frame. +//! +//! Run: +//! RUSTFLAGS="-C target-cpu=x86-64-v3" \ +//! cargo run --release --features bench --example bench_round29_micro +//! Tunables (env): A_ROWS (500), B_ITERS (200000), C_ITERS (50000), C_FRAME (200000) + +use std::alloc::{GlobalAlloc, Layout, System}; +use std::env; +use std::hint::black_box; +use std::sync::Arc; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::time::Instant; + +use bytes::{Bytes, BytesMut}; + +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) +} + +fn measure(iters: u64, mut f: impl FnMut()) -> (f64, f64) { + f(); + ALLOC_CALLS.store(0, Ordering::Relaxed); + let start = Instant::now(); + for _ in 0..iters { + f(); + } + let ns = start.elapsed().as_nanos() as f64 / iters as f64; + let allocs = ALLOC_CALLS.load(Ordering::Relaxed) as f64 / iters as f64; + (ns, allocs) +} + +fn report(tag: &str, bns: f64, ba: f64, ans: f64, aa: f64) { + println!("## {tag}"); + println!("| arm | ns/op | allocs/op |"); + println!("| BEFORE | {bns:>9.1} | {ba:>9.2} |"); + println!("| AFTER | {ans:>9.1} | {aa:>9.2} |"); + println!( + "# {:.2}x wall · {:.2} fewer allocs/op\n", + bns / ans.max(0.0001), + ba - aa + ); +} + +fn gate(tag: &str, before: f64, after: f64) { + if after >= before { + eprintln!("GATE FAIL [{tag}] allocs/op: AFTER {after} !< BEFORE {before} — rollback"); + std::process::exit(1); + } +} + +// ── [A] NextCloud REPORT href: per-row String(s) vs reused buffer ───────────── +// Faithful replicas of the source functions. +fn nc_href(username: &str, subpath: &str) -> String { + let subpath = subpath.trim_matches('/'); + let encoded_user = urlencoding::encode(username); + const PREFIX: &str = "/remote.php/dav/files/"; + let mut out = String::with_capacity(PREFIX.len() + encoded_user.len() + subpath.len() + 8); + out.push_str(PREFIX); + out.push_str(&encoded_user); + out.push('/'); + for (i, seg) in subpath.split('/').enumerate() { + if i > 0 { + out.push('/'); + } + out.push_str(&urlencoding::encode(seg)); + } + out +} + +/// AFTER: write the href into a reused buffer with a precomputed encoded user. +fn nc_href_into(out: &mut String, encoded_user: &str, subpath: &str) { + let subpath = subpath.trim_matches('/'); + out.clear(); + const PREFIX: &str = "/remote.php/dav/files/"; + out.reserve(PREFIX.len() + encoded_user.len() + subpath.len() + 8); + out.push_str(PREFIX); + out.push_str(encoded_user); + out.push('/'); + for (i, seg) in subpath.split('/').enumerate() { + if i > 0 { + out.push('/'); + } + out.push_str(&urlencoding::encode(seg)); + } +} + +/// AFTER: collection variant — trailing slash guaranteed, in place. +fn nc_collection_href_into(out: &mut String, encoded_user: &str, subpath: &str) { + nc_href_into(out, encoded_user, subpath); + if !out.ends_with('/') { + out.push('/'); + } +} + +fn section_a() { + let rows: usize = env_or("A_ROWS", 500); + let user = "admin"; + // A flat REPORT/search result: files and folders at varying paths (each row + // a DIFFERENT subpath, unlike PROPFIND's shared-parent children — so the win + // is the per-row String + the once-per-page user encode, not a hoisted prefix). + let paths: Vec<(bool, String)> = (0..rows) + .map(|i| { + let is_dir = i % 2 == 0; + let p = format!("Documents/2024/q{}/report-{i}.dat", i % 4); + (is_dir, p) + }) + .collect(); + + // Equivalence: AFTER href bytes match BEFORE for every row. + { + let encoded_user = urlencoding::encode(user); + let mut buf = String::new(); + for (is_dir, p) in &paths { + let before = if *is_dir { + format!("{}/", nc_href(user, p)) + } else { + nc_href(user, p) + }; + if *is_dir { + nc_collection_href_into(&mut buf, &encoded_user, p); + } else { + nc_href_into(&mut buf, &encoded_user, p); + } + assert_eq!(buf, before, "A href differs for {p}"); + } + } + + let (bns, ba) = measure(2000, || { + // BEFORE: nc_href per file row; nc_href + format! per folder row. + let mut sink = 0usize; + for (is_dir, p) in &paths { + let href = if *is_dir { + format!("{}/", nc_href(user, black_box(p))) + } else { + nc_href(user, black_box(p)) + }; + sink += href.len(); + } + black_box(sink); + }); + let (ans, aa) = measure(2000, || { + // AFTER: one reused buffer, user encoded once per page. + let encoded_user = urlencoding::encode(user); + let mut href = String::new(); + let mut sink = 0usize; + for (is_dir, p) in &paths { + if *is_dir { + nc_collection_href_into(&mut href, &encoded_user, black_box(p)); + } else { + nc_href_into(&mut href, &encoded_user, black_box(p)); + } + sink += href.len(); + } + black_box(sink); + }); + report( + &format!("[A] NC REPORT href ({rows} rows)"), + bns, + ba, + ans, + aa, + ); + gate("A", ba, aa); +} + +// ── [B] cache-serve fast path: eager owned args vs borrow-probe ─────────────── +fn section_b() { + let iters: u64 = env_or("B_ITERS", 200_000); + let hash = "b3a1c0ffee1234567890abcdef0123456789abcdef0123456789abcdef012345"; + let id = "3f2504e0-4f89-41d3-9a0c-0305e82c3301"; + let mime: Arc = Arc::from("video/mp4"); + // A tiny content-addressed "cache": key = blob hash → (bytes, etag, ct). + let mut map: std::collections::HashMap, Arc)> = + std::collections::HashMap::new(); + let etag_stored: Arc = format!("\"{hash}\"").into(); + map.insert( + hash.to_string(), + ( + Bytes::from_static(b"\x00\x01\x02\x03some-cached-blob-bytes"), + etag_stored, + mime.clone(), + ), + ); + + // Equivalence: both arms retrieve the identical cached Bytes on a hit. + let before_hit = { + let _etag: Arc = format!("\"{hash}\"").into(); + let _key = hash.to_string(); + let _id_owned = id.to_string(); + map.get(hash).map(|(b, _, _)| b.clone()) + }; + let after_hit = map.get(hash).map(|(b, _, _)| b.clone()); + assert_eq!(before_hit, after_hit, "B cached bytes differ"); + + let (bns, ba) = measure(iters, || { + // BEFORE: build the owned get_or_load args, THEN probe (hit ignores them). + let etag: Arc = format!("\"{}\"", black_box(hash)).into(); + let ct: Arc = mime.clone(); + let id_owned = black_box(id).to_string(); + let key = black_box(hash).to_string(); + let hit = map.get(key.as_str()).map(|(b, _, _)| b.clone()); + black_box((etag, ct, id_owned, hit)); + }); + let (ans, aa) = measure(iters, || { + // AFTER: probe with a borrow first; on a hit build nothing. + let hit = map.get(black_box(hash)).map(|(b, _, _)| b.clone()); + black_box(hit); + }); + report("[B] cache-serve fast path (hit)", bns, ba, ans, aa); + gate("B", ba, aa); +} + +// ── [C] read_full: single-frame BytesMut concat vs zero-copy passthrough ────── +fn read_full_before(frames: &[Bytes], capacity: usize) -> Bytes { + let mut buf = BytesMut::with_capacity(capacity); + for f in frames { + buf.extend_from_slice(f); + } + buf.freeze() +} + +fn read_full_after(frames: &[Bytes], capacity: usize) -> Bytes { + // Single frame → return it directly (zero copy). Multi-frame → identical concat. + match frames { + [] => Bytes::new(), + [only] => only.clone(), + _ => { + let mut buf = BytesMut::with_capacity(capacity); + for f in frames { + buf.extend_from_slice(f); + } + buf.freeze() + } + } +} + +fn section_c() { + let iters: u64 = env_or("C_ITERS", 50_000); + let frame_len: usize = env_or("C_FRAME", 200_000); + // The local backend yields one owned contiguous frame for a ≤256 KB file. + let frame = Bytes::from(vec![0u8; frame_len]); + let frames = [frame.clone()]; + let cap = frame_len; + + // Equivalence: identical bytes out. + assert_eq!( + read_full_before(&frames, cap), + read_full_after(&frames, cap), + "C single-frame bytes differ" + ); + + let (bns, ba) = measure(iters, || { + black_box(read_full_before(black_box(&frames), cap)); + }); + let (ans, aa) = measure(iters, || { + black_box(read_full_after(black_box(&frames), cap)); + }); + report( + &format!("[C] read_full single frame ({frame_len} B)"), + bns, + ba, + ans, + aa, + ); + gate("C", ba, aa); +} + +// ── [D] login-lockout key: to_lowercase()+format! vs single ASCII buffer ────── +fn lockout_key_before(username: &str, client_ip: &str) -> String { + format!("{}|{}", username.to_lowercase(), client_ip) +} +fn lockout_key_after(username: &str, client_ip: &str) -> String { + if username.is_ascii() { + let mut k = String::with_capacity(username.len() + 1 + client_ip.len()); + for &b in username.as_bytes() { + k.push(b.to_ascii_lowercase() as char); + } + k.push('|'); + k.push_str(client_ip); + k + } else { + format!("{}|{}", username.to_lowercase(), client_ip) + } +} + +fn section_d() { + let iters: u64 = env_or("D_ITERS", 200_000); + let username = "alice.app-password"; + let client_ip = "203.0.113.42"; + // Equivalence across a matrix incl. mixed-case, composite marker, non-ASCII. + for (u, ip) in [ + ("alice", "1.2.3.4"), + ("Alice.Smith", "203.0.113.42"), + ("BOB", "::1"), + ("home~a1b2", "10.0.0.1"), + ("ünïcode", "2001:db8::1"), + ("", "unknown"), + ] { + assert_eq!( + lockout_key_before(u, ip), + lockout_key_after(u, ip), + "D key differs for {u}" + ); + } + let (bns, ba) = measure(iters, || { + black_box(lockout_key_before( + black_box(username), + black_box(client_ip), + )); + }); + let (ans, aa) = measure(iters, || { + black_box(lockout_key_after(black_box(username), black_box(client_ip))); + }); + report("[D] login-lockout key (ASCII)", bns, ba, ans, aa); + gate("D", ba, aa); +} + +// ── [E] NC composite-username parse: owned clone/to_string vs borrow ────────── +fn section_e() { + let iters: u64 = env_or("E_ITERS", 500_000); + let raw_no_marker = "alice.app-password".to_string(); + let raw_marker = "alice~a1b2c3d4".to_string(); + // Equivalence: borrowed slices equal the owned versions. + { + let (bu, bm): (&str, Option<&str>) = match raw_no_marker.split_once('~') { + Some((u, m)) => (u, Some(m)), + None => (raw_no_marker.as_str(), None), + }; + assert_eq!(bu, raw_no_marker.as_str()); + assert!(bm.is_none()); + let (mu, mm) = raw_marker.split_once('~').unwrap(); + assert_eq!((mu, mm), ("alice", "a1b2c3d4")); + } + let (bns, ba) = measure(iters, || { + // BEFORE: the no-marker path clones raw_username into an owned String. + let (username, drive_marker): (String, Option) = + match black_box(&raw_no_marker).split_once('~') { + Some((u, m)) => (u.to_string(), Some(m.to_string())), + None => (raw_no_marker.clone(), None), + }; + black_box((username, drive_marker)); + }); + let (ans, aa) = measure(iters, || { + // AFTER: borrow the slices out of the already-owned raw_username. + let (username, drive_marker): (&str, Option<&str>) = + match black_box(&raw_no_marker).split_once('~') { + Some((u, m)) => (u, Some(m)), + None => (raw_no_marker.as_str(), None), + }; + black_box((username, drive_marker)); + }); + report("[E] NC username parse (no-marker)", bns, ba, ans, aa); + gate("E", ba, aa); +} + +// ── [F] contact-group listing: decode the discarded vcard String vs skip it ─── +fn section_f() { + let rows: usize = env_or("F_ROWS", 200); + let vcard_len: usize = env_or("F_VCARD", 8192); // ~8 KiB with an embedded base64 PHOTO + let vcard_src = vec![b'v'; vcard_len]; + let (bns, ba) = measure(200, || { + // BEFORE: decode the vcard TEXT column into an owned String per row, + // then discard it (ContactDto has no vcard field). + let mut sink = 0usize; + for _ in 0..rows { + let vcard = String::from_utf8(black_box(&vcard_src).clone()).unwrap(); + sink += vcard.len(); + } + black_box(sink); + }); + let (ans, aa) = measure(200, || { + // AFTER: column not selected → empty String, no per-row alloc/copy. + let mut sink = 0usize; + for _ in 0..rows { + let vcard = String::new(); + sink += vcard.len(); + } + black_box(sink); + }); + report( + &format!("[F] contact-group vcard over-fetch ({rows}×{vcard_len}B)"), + bns, + ba, + ans, + aa, + ); + gate("F", ba, aa); +} + +// ── [G] admin count: hydrate N full user rows vs scalar COUNT ────────────────── +struct FakeUser { + _username: String, + _image: String, // avatar data URI (server allows up to 512 KiB) + _prefs: serde_json::Value, // ui_preferences JSONB DOM +} + +fn section_g() { + let admins: usize = env_or("G_ADMINS", 3); + let image_len: usize = env_or("G_IMAGE", 65_536); // 64 KiB avatar (up to 512 KiB allowed) + let image_src = vec![b'i'; image_len]; + let prefs_json = r#"{"theme":"dark","density":"comfortable","sidebar":true}"#; + let (bns, ba) = measure(2000, || { + // BEFORE: hydrate every admin's full row (username + avatar String + + // ui_preferences Value DOM) only to take the count. + let users: Vec = (0..admins) + .map(|i| FakeUser { + _username: format!("admin{i}"), + _image: String::from_utf8(black_box(&image_src).clone()).unwrap(), + _prefs: serde_json::from_str(black_box(prefs_json)).unwrap(), + }) + .collect(); + black_box(users.len() as i64); + }); + let (ans, aa) = measure(2000, || { + // AFTER: a scalar count — no rows hydrated. + let count: i64 = black_box(admins) as i64; + black_box(count); + }); + report( + &format!("[G] admin-count hydrate vs COUNT ({admins} admins × {image_len}B avatar)"), + bns, + ba, + ans, + aa, + ); + gate("G", ba, aa); +} + +fn main() { + println!("# Round-29 micro alloc pack\n"); + section_a(); + section_b(); + section_c(); + section_d(); + section_e(); + section_f(); + section_g(); + println!("All Round-29 micro sections passed their gate."); +} diff --git a/src/application/ports/auth_ports.rs b/src/application/ports/auth_ports.rs index 6b680518..b4c2e3f1 100644 --- a/src/application/ports/auth_ports.rs +++ b/src/application/ports/auth_ports.rs @@ -172,6 +172,12 @@ pub trait UserStoragePort: Send + Sync + 'static { /// Lists users by role (e.g., "admin" or "user") async fn list_users_by_role(&self, role: &str) -> Result, DomainError>; + /// Counts users with a given role WITHOUT hydrating their rows — a scalar + /// `COUNT(*)` instead of fetching every full user row (incl. the up-to-512 + /// KiB avatar `image` and the `ui_preferences` JSONB) only to `.len()` them + /// (benches/ROUND29.md §G). + async fn count_users_by_role(&self, role: &str) -> Result; + /// Deletes a user by their ID async fn delete_user(&self, user_id: Uuid) -> Result<(), DomainError>; diff --git a/src/application/services/auth_application_service.rs b/src/application/services/auth_application_service.rs index 87befc00..c5091e6b 100644 --- a/src/application/services/auth_application_service.rs +++ b/src/application/services/auth_application_service.rs @@ -2118,21 +2118,11 @@ impl AuthApplicationService { // Method to count how many admin users exist in the system // Used to determine if we have multiple admins or just the default one pub async fn count_admin_users(&self) -> Result { - // Use the list_users_by_role method or similar from user_storage port - // For now, we'll use a basic implementation that counts all users with role = "admin" - let admin_users = self - .user_storage - .list_users_by_role("admin") - .await - .map_err(|e| { - DomainError::new( - ErrorKind::InternalError, - "User", - format!("Error counting admin users: {}", e), - ) - })?; - - Ok(admin_users.len() as i64) + // Scalar COUNT(*) — the old form fetched every admin's FULL row (incl. + // the up-to-512 KiB avatar `image` + `ui_preferences` JSONB) only to + // call `.len()`, on a status/init endpoint that is polled at bootstrap + // (benches/ROUND29.md §G). + self.user_storage.count_users_by_role("admin").await } /// Lists internal users only. External (grant-only) users are filtered diff --git a/src/application/services/file_retrieval_service.rs b/src/application/services/file_retrieval_service.rs index b0262e28..e39fb27b 100644 --- a/src/application/services/file_retrieval_service.rs +++ b/src/application/services/file_retrieval_service.rs @@ -111,7 +111,26 @@ impl FileRetrievalService { ) -> Result { let stream = file_read.get_file_stream(id).await?; let mut stream = Pin::from(stream); - let mut buf = BytesMut::with_capacity(capacity); + // Most sub-threshold reads arrive as ONE owned contiguous frame from the + // backend (the local ReaderStream emits ≤256 KiB frames, and a + // sub-threshold blob fits in one). Return that frame directly instead of + // copying the whole payload a second time into a fresh BytesMut; only a + // multi-frame read pays the pre-sized concat — byte-identical output + // (benches/ROUND29.md §C). + let Some(first) = stream.next().await else { + return Ok(Bytes::new()); + }; + let first = first.map_err(|e| { + DomainError::internal_error("File", format!("Stream read error: {}", e)) + })?; + let Some(second) = stream.next().await else { + return Ok(first); + }; + let mut buf = BytesMut::with_capacity(capacity.max(first.len())); + buf.extend_from_slice(&first); + buf.extend_from_slice(&second.map_err(|e| { + DomainError::internal_error("File", format!("Stream read error: {}", e)) + })?); while let Some(chunk) = stream.next().await { buf.extend_from_slice(&chunk.map_err(|e| { DomainError::internal_error("File", format!("Stream read error: {}", e)) @@ -202,7 +221,6 @@ impl FileRetrievalService { ) -> Result<(FileDto, OptimizedFileContent), DomainError> { let mime_type = dto.mime_type.clone(); let file_size = dto.size; - let file_name = dto.name.clone(); // The content cache is content-addressed: keyed by the blob hash, not // the file id. Identical content deduplicated to one blob on disk is // then cached ONCE in RAM and shared by every file/user that references @@ -210,34 +228,39 @@ impl FileRetrievalService { // construction, so entries never go stale (no invalidation needed). A // stub DTO without a hash disables caching for that request rather than // colliding every hash-less file on the key "". - let cache_key = dto.content_hash.clone(); - let cacheable = !cache_key.is_empty(); + let cacheable = !dto.content_hash.is_empty(); let do_transcode = accept_webp && !prefer_original; // ── Tier 1: Hot cache + transcode (<10 MB) ────────── if file_size < CACHE_THRESHOLD { - // Fetch the raw blob bytes. When cacheable, `get_or_load` serves - // from the content cache on a hit and, on a miss, coalesces every - // concurrent request for the same blob hash into a SINGLE disk read - // (single-flight) — no thundering herd under load. Hash-less stub - // DTOs are uncacheable and stream straight from disk. + // Probe the content cache with a BORROW first: a hit serves the blob + // straight from RAM, and only a miss builds the owned load arguments + // (the quoted-etag / key / id Strings) that a hit would otherwise + // allocate and immediately discard (benches/ROUND29.md §B). On a miss + // `load_and_cache` still coalesces concurrent requests for the same + // blob hash into a SINGLE disk read (single-flight) — no thundering + // herd. Hash-less stub DTOs are uncacheable and stream from disk. let content_bytes = if cacheable && let Some(cache) = &self.content_cache { - let etag: Arc = format!("\"{}\"", cache_key).into(); - let ct: Arc = mime_type.clone(); - let file_read = Arc::clone(&self.file_read); - let id_owned = id.to_string(); - let cap = file_size as usize; - let (bytes, _etag, _ct) = cache - .get_or_load(cache_key.clone(), etag, ct, async move { - debug!("💾 TIER 1 Cache MISS: {} – loading from disk", id_owned); - Self::read_full(&file_read, &id_owned, cap).await - }) - .await?; - bytes + if let Some((bytes, ..)) = cache.get(&dto.content_hash).await { + bytes + } else { + let etag: Arc = format!("\"{}\"", dto.content_hash).into(); + let ct: Arc = mime_type.clone(); + let file_read = Arc::clone(&self.file_read); + let id_owned = id.to_string(); + let cap = file_size as usize; + let (bytes, ..) = cache + .load_and_cache(dto.content_hash.to_string(), etag, ct, async move { + debug!("💾 TIER 1 Cache MISS: {} – loading from disk", id_owned); + Self::read_full(&file_read, &id_owned, cap).await + }) + .await?; + bytes + } } else { debug!( "💾 TIER 1 (uncacheable): {} – streaming from disk", - file_name + dto.name ); Self::read_full(&self.file_read, id, file_size as usize).await? }; @@ -269,7 +292,7 @@ impl FileRetrievalService { // ── Tier 2 + 3: Streaming (≥10 MB) ────────────────── info!( "📡 TIER 2 STREAMING: {} ({} MB)", - file_name, + dto.name, file_size / (1024 * 1024) ); let stream = self.file_read.get_file_stream(id).await?; @@ -353,17 +376,27 @@ impl FileRetrievalService { ) -> Result { let cacheable = dto.size < CACHE_THRESHOLD && !dto.content_hash.is_empty(); if cacheable && let Some(cache) = &self.content_cache { - let etag: Arc = format!("\"{}\"", dto.content_hash).into(); - let ct: Arc = dto.mime_type.clone(); - let file_read = Arc::clone(&self.file_read); - let id_owned = dto.id.clone(); - let cap = dto.size as usize; - let (bytes, _etag, _ct) = cache - .get_or_load(dto.content_hash.to_string(), etag, ct, async move { - debug!("💾 Range cache MISS: {} – loading from disk", id_owned); - Self::read_full(&file_read, &id_owned, cap).await - }) - .await?; + // Probe with a BORROW first: the video-scrub steady state is a cache + // hit, and a hit must not allocate the owned load args (quoted-etag / + // key / id Strings) it would immediately discard — those are built + // only on the miss branch (benches/ROUND29.md §B). A miss still + // populates via the same single-flight coalescing. + let bytes = if let Some((bytes, ..)) = cache.get(&dto.content_hash).await { + bytes + } else { + let etag: Arc = format!("\"{}\"", dto.content_hash).into(); + let ct: Arc = dto.mime_type.clone(); + let file_read = Arc::clone(&self.file_read); + let id_owned = dto.id.clone(); + let cap = dto.size as usize; + let (bytes, ..) = cache + .load_and_cache(dto.content_hash.to_string(), etag, ct, async move { + debug!("💾 Range cache MISS: {} – loading from disk", id_owned); + Self::read_full(&file_read, &id_owned, cap).await + }) + .await?; + bytes + }; let len = bytes.len() as u64; let s = start.min(len) as usize; let e = end.unwrap_or(len).min(len) as usize; diff --git a/src/domain/repositories/user_repository.rs b/src/domain/repositories/user_repository.rs index d129f2d1..d2ecb604 100644 --- a/src/domain/repositories/user_repository.rs +++ b/src/domain/repositories/user_repository.rs @@ -111,6 +111,10 @@ pub trait UserRepository: Send + Sync + 'static { /// Lists users by role (admin or user) async fn list_users_by_role(&self, role: &str) -> UserRepositoryResult>; + /// Counts users with a given role via a scalar `COUNT(*)` — no row + /// hydration (benches/ROUND29.md §G). + async fn count_users_by_role(&self, role: &str) -> UserRepositoryResult; + /// Deletes a user async fn delete_user(&self, user_id: Uuid) -> UserRepositoryResult<()>; diff --git a/src/infrastructure/repositories/pg/contact_group_pg_repository.rs b/src/infrastructure/repositories/pg/contact_group_pg_repository.rs index 945d7d1e..02383cc0 100644 --- a/src/infrastructure/repositories/pg/contact_group_pg_repository.rs +++ b/src/infrastructure/repositories/pg/contact_group_pg_repository.rs @@ -196,10 +196,10 @@ impl ContactGroupRepository for ContactGroupPgRepository { ) -> ContactRepositoryResult> { let rows = sqlx::query( r#" - SELECT + SELECT c.id, c.address_book_id, c.uid, c.full_name, c.first_name, c.last_name, c.nickname, c.email, c.phone, c.address, c.organization, c.title, c.notes, c.photo_url, - c.birthday, c.anniversary, c.vcard, c.etag, c.created_at, c.updated_at + c.birthday, c.anniversary, c.etag, c.created_at, c.updated_at FROM carddav.contacts c INNER JOIN carddav.group_memberships gm ON c.id = gm.contact_id WHERE gm.group_id = $1 @@ -253,7 +253,13 @@ impl ContactGroupRepository for ContactGroupPgRepository { row.get::, _>("photo_url"), row.get("birthday"), row.get("anniversary"), - row.get("vcard"), + // vcard column intentionally NOT selected — the sole live caller + // (`list_contacts_in_group`) maps to `ContactDto`, which has no + // vcard field, so fetching the multi-KB serialized vCard (with an + // embedded base64 PHOTO) only to drop it wastes bandwidth + a + // per-row String. Mirrors `row_to_contact_lite` (benches/ROUND29.md + // §F / ROUND25 §Q2, applied to the LIVE group method this time). + String::new(), row.get("etag"), row.get("created_at"), row.get("updated_at"), diff --git a/src/infrastructure/repositories/pg/user_pg_repository.rs b/src/infrastructure/repositories/pg/user_pg_repository.rs index ae216446..1dcfd11e 100644 --- a/src/infrastructure/repositories/pg/user_pg_repository.rs +++ b/src/infrastructure/repositories/pg/user_pg_repository.rs @@ -810,6 +810,15 @@ impl UserRepository for UserPgRepository { Ok(()) } + /// Counts users by role with a scalar `COUNT(*)` — no row hydration. + async fn count_users_by_role(&self, role: &str) -> UserRepositoryResult { + sqlx::query_scalar("SELECT COUNT(*) FROM auth.users WHERE role::text = $1") + .bind(role) + .fetch_one(&*self.pool) + .await + .map_err(Self::map_sqlx_error) + } + /// Lists users by role async fn list_users_by_role(&self, role: &str) -> UserRepositoryResult> { let rows = sqlx::query( @@ -1157,6 +1166,12 @@ impl UserStoragePort for UserPgRepository { .map_err(DomainError::from) } + async fn count_users_by_role(&self, role: &str) -> Result { + UserRepository::count_users_by_role(self, role) + .await + .map_err(DomainError::from) + } + async fn delete_user(&self, user_id: Uuid) -> Result<(), DomainError> { UserRepository::delete_user(self, user_id) .await diff --git a/src/infrastructure/services/file_content_cache.rs b/src/infrastructure/services/file_content_cache.rs index 5c6ee555..269e8c6e 100644 --- a/src/infrastructure/services/file_content_cache.rs +++ b/src/infrastructure/services/file_content_cache.rs @@ -182,7 +182,30 @@ impl FileContentCache { if let Some(hit) = self.get(&cache_key).await { return Ok(hit); } + self.load_and_cache(cache_key, etag, content_type, load) + .await + } + /// The populate-on-miss half of [`Self::get_or_load`], with single-flight + /// coalescing but WITHOUT the leading `get` probe. + /// + /// Hot read paths that have *already* probed the cache with [`Self::get`] + /// (a borrow) call this directly on the miss branch — they then build the + /// owned `cache_key` / `etag` / `content_type` (each a heap allocation) + /// only when they are actually needed to populate, so a cache HIT allocates + /// none of them (benches/ROUND29.md §B). Because the caller's own `get` + /// already counted the hit/miss, this method does not re-probe — keeping the + /// hit/miss stat counts identical to a single `get_or_load` call. + pub async fn load_and_cache( + &self, + cache_key: String, + etag: Arc, + content_type: Arc, + load: F, + ) -> Result<(Bytes, Arc, Arc), DomainError> + where + F: Future>, + { // Slow path: coalesce concurrent misses into a single `load`. let entry = self .cache diff --git a/src/infrastructure/services/login_lockout_service.rs b/src/infrastructure/services/login_lockout_service.rs index 47916909..1c0a3e14 100644 --- a/src/infrastructure/services/login_lockout_service.rs +++ b/src/infrastructure/services/login_lockout_service.rs @@ -68,7 +68,24 @@ impl LoginLockoutService { fn key(username: &str, client_ip: &str) -> String { // `|` is not valid in either a username or an IP literal so it makes // the username/ip boundary unambiguous. - format!("{}|{}", username.to_lowercase(), client_ip) + // + // The lowercased composite is written into ONE pre-sized buffer instead + // of the `to_lowercase()` (alloc) + `format!` (alloc) two-step. App + // passwords authenticate with an already-lowercase ASCII username in + // ~all traffic, so the fast branch covers it; the rare non-ASCII branch + // keeps `str::to_lowercase` for exact Unicode (e.g. final-sigma) + // semantics. Byte-identical key either way (benches/ROUND29.md §D). + if username.is_ascii() { + let mut k = String::with_capacity(username.len() + 1 + client_ip.len()); + for &b in username.as_bytes() { + k.push(b.to_ascii_lowercase() as char); + } + k.push('|'); + k.push_str(client_ip); + k + } else { + format!("{}|{}", username.to_lowercase(), client_ip) + } } /// Check whether the (account, IP) pair is currently locked. diff --git a/src/interfaces/nextcloud/basic_auth_middleware.rs b/src/interfaces/nextcloud/basic_auth_middleware.rs index 765f7ad1..7e088f17 100644 --- a/src/interfaces/nextcloud/basic_auth_middleware.rs +++ b/src/interfaces/nextcloud/basic_auth_middleware.rs @@ -109,7 +109,12 @@ pub async fn basic_auth_middleware( // at the auth boundary rather than treating them as "missing // marker" — they are unambiguous typos that would otherwise // silently fall into a different code path. - let (username, drive_marker): (String, Option) = match raw_username.split_once('~') { + // Borrow the prefix / marker out of the already-owned `raw_username` + // (`split_once` yields `&str` slices) instead of allocating a duplicate + // `String` per request — `username` is only ever passed by reference, and + // `raw_username` outlives every use before it moves into `NcSession` + // (benches/ROUND29.md §E). + let (username, drive_marker): (&str, Option<&str>) = match raw_username.split_once('~') { Some(("", _)) => { tracing::warn!( "[NC] 401 malformed composite username (empty prefix): {}", @@ -124,15 +129,15 @@ pub async fn basic_auth_middleware( ); return Err(NextcloudAuthError::Unauthorized); } - Some((u, m)) => (u.to_string(), Some(m.to_string())), - None => (raw_username.clone(), None), + Some((u, m)) => (u, Some(m)), + None => (raw_username.as_str(), None), }; // Check account lockout before attempting password verification (saves CPU). // The lockout is per (account, IP), see #323 for rationale. let client_ip = crate::interfaces::middleware::rate_limit::extract_client_ip(&request); if let Some(auth_svc) = state.auth_service.as_ref() - && let Err(secs) = auth_svc.login_lockout.check(&username, &client_ip) + && let Err(secs) = auth_svc.login_lockout.check(username, &client_ip) { tracing::warn!( username = %username, @@ -150,13 +155,13 @@ pub async fn basic_auth_middleware( match nextcloud .app_passwords - .verify_basic_auth(&username, &password) + .verify_basic_auth(username, &password) .await { Ok((user_id, uname, email, role)) => { // Reset lockout counter on success if let Some(auth_svc) = state.auth_service.as_ref() { - auth_svc.login_lockout.record_success(&username, &client_ip); + auth_svc.login_lockout.record_success(username, &client_ip); } // External users must never authenticate against the NC // surface — that whole subtree (WebDAV files, uploads, @@ -222,7 +227,7 @@ pub async fn basic_auth_middleware( // is the right one: name-independent, secondary-drive-safe. use crate::application::ports::folder_ports::FolderUseCase; use crate::domain::repositories::drive_repository::DriveRepository; - let chroot = match drive_marker.as_deref() { + let chroot = match drive_marker { None => { match state .drive_repo @@ -287,7 +292,7 @@ pub async fn basic_auth_middleware( Err(_) => { // Record failed attempt for lockout tracking if let Some(auth_svc) = state.auth_service.as_ref() { - auth_svc.login_lockout.record_failure(&username, &client_ip); + auth_svc.login_lockout.record_failure(username, &client_ip); } Err(NextcloudAuthError::Unauthorized) } diff --git a/src/interfaces/nextcloud/report_handler.rs b/src/interfaces/nextcloud/report_handler.rs index 5d1c6a76..8c6f91ae 100644 --- a/src/interfaces/nextcloud/report_handler.rs +++ b/src/interfaces/nextcloud/report_handler.rs @@ -24,8 +24,8 @@ use crate::interfaces::api::handlers::webdav_handler::{ }; use crate::interfaces::errors::AppError; use crate::interfaces::nextcloud::webdav_handler::{ - batch_resolve_ids, format_oc_id_into, nc_href, nc_id_of, write_file_response, - write_folder_response, + batch_resolve_ids, format_oc_id_into, nc_collection_href_into, nc_href_into, nc_id_of, + write_file_response, write_folder_response, }; /// Handle WebDAV REPORT and SEARCH methods for Nextcloud compatibility. @@ -177,6 +177,12 @@ async fn handle_filter_files( // owner-id stays canonical via `&user.username`. // One oc:id buffer reused across both emit loops (benches/ROUND27.md §H1). let mut oc_buf = String::new(); + // One href buffer reused across both emit loops, with the URL-encoded + // user computed once for the page instead of re-encoded per row — the + // reused-buffer shape the PROPFIND child loop already uses + // (benches/ROUND29.md §A). + let encoded_user = urlencoding::encode(url_user); + let mut href_buf = String::new(); for file in &files { // Skip favorites that live outside the caller's chroot // (other-drive favorites); reachable via REST if needed. @@ -189,7 +195,7 @@ async fn handle_filter_files( ); continue; }; - let href = nc_href(url_user, subpath); + nc_href_into(&mut href_buf, &encoded_user, subpath); let fid = nc_id_of(&file_id_map, &file.id); let oc_id: Option<&str> = match fid { Some(id) => { @@ -202,7 +208,7 @@ async fn handle_filter_files( write_file_response( &mut xml, file, - &href, + &href_buf, (fid, oc_id), &user.username, &favorite_ids, @@ -221,7 +227,7 @@ async fn handle_filter_files( ); continue; }; - let href = format!("{}/", nc_href(url_user, subpath)); + nc_collection_href_into(&mut href_buf, &encoded_user, subpath); let fid = nc_id_of(&folder_id_map, &folder.id); let oc_id: Option<&str> = match fid { Some(id) => { @@ -234,7 +240,7 @@ async fn handle_filter_files( write_folder_response( &mut xml, folder, - &href, + &href_buf, (fid, oc_id), &user.username, &favorite_ids, @@ -334,6 +340,12 @@ async fn handle_search( // Files. // One oc:id buffer reused across both emit loops (benches/ROUND27.md §H1). let mut oc_buf = String::new(); + // One href buffer reused across both emit loops, with the URL-encoded + // user computed once for the page instead of re-encoded per row — the + // reused-buffer shape the PROPFIND child loop already uses + // (benches/ROUND29.md §A). + let encoded_user = urlencoding::encode(url_user); + let mut href_buf = String::new(); for file in &files { let Some(subpath) = strip_home_prefix(chroot, &file.path, home_prefix) else { tracing::debug!( @@ -344,7 +356,7 @@ async fn handle_search( ); continue; }; - let href = nc_href(url_user, subpath); + nc_href_into(&mut href_buf, &encoded_user, subpath); let fid = nc_id_of(&file_id_map, &file.id); let oc_id: Option<&str> = match fid { Some(id) => { @@ -357,7 +369,7 @@ async fn handle_search( write_file_response( &mut xml, file, - &href, + &href_buf, (fid, oc_id), &user.username, &favorite_ids, @@ -377,7 +389,7 @@ async fn handle_search( ); continue; }; - let href = format!("{}/", nc_href(url_user, subpath)); + nc_collection_href_into(&mut href_buf, &encoded_user, subpath); let fid = nc_id_of(&folder_id_map, &folder.id); let oc_id: Option<&str> = match fid { Some(id) => { @@ -390,7 +402,7 @@ async fn handle_search( write_folder_response( &mut xml, folder, - &href, + &href_buf, (fid, oc_id), &user.username, &favorite_ids, diff --git a/src/interfaces/nextcloud/webdav_handler.rs b/src/interfaces/nextcloud/webdav_handler.rs index ea8abfbd..721dd7f6 100644 --- a/src/interfaces/nextcloud/webdav_handler.rs +++ b/src/interfaces/nextcloud/webdav_handler.rs @@ -167,12 +167,10 @@ pub fn strip_drive_root_segment(internal_path: &str) -> &str { /// surfaces as `Network request error "Erreur inconnue" HTTP status /// 207` in the client log. Files use [`nc_href`] (no trailing slash). pub fn nc_collection_href(username: &str, subpath: &str) -> String { - let h = nc_href(username, subpath); - if h.ends_with('/') { - h - } else { - format!("{}/", h) - } + let encoded_user = urlencoding::encode(username); + let mut out = String::new(); + nc_collection_href_into(&mut out, &encoded_user, subpath); + out } /// Build the Nextcloud DAV href for a resource. @@ -184,17 +182,33 @@ pub fn nc_collection_href(username: &str, subpath: &str) -> String { /// a **collection** must use [`nc_collection_href`] (or append `/` /// manually) to satisfy RFC 4918 §5.2 and the NC client's parser. pub fn nc_href(username: &str, subpath: &str) -> String { - let subpath = subpath.trim_matches('/'); let encoded_user = urlencoding::encode(username); + let mut out = String::new(); + nc_href_into(&mut out, &encoded_user, subpath); + out +} + +/// Per-row form of [`nc_href`]: write the href into a REUSED buffer given the +/// already-URL-encoded username. +/// +/// The emit loops (PROPFIND children, REPORT results) call this instead of +/// [`nc_href`] so each row rewrites one buffer rather than allocating a fresh +/// `String`, and the constant `encoded_user` is encoded ONCE per page instead of +/// re-encoded for every row (benches/ROUND29.md §A — the same reused-buffer shape +/// the PROPFIND child loop already uses for its href prefix). Byte-identical to +/// [`nc_href`]. +pub fn nc_href_into(out: &mut String, encoded_user: &str, subpath: &str) { + let subpath = subpath.trim_matches('/'); // Write the prefix, user and each encoded segment straight into one // pre-sized buffer — avoids the per-segment `Vec`, the joined String and // the `format!` result the previous `.map(...).collect().join("/")` allocated // on every NC PROPFIND/REPORT href (mirrors the native `encode_uri_path`). // Keeps `urlencoding::encode` so the emitted bytes are unchanged. const PREFIX: &str = "/remote.php/dav/files/"; - let mut out = String::with_capacity(PREFIX.len() + encoded_user.len() + subpath.len() + 8); + out.clear(); + out.reserve(PREFIX.len() + encoded_user.len() + subpath.len() + 8); out.push_str(PREFIX); - out.push_str(&encoded_user); + out.push_str(encoded_user); out.push('/'); // No empty-segment filter: `split('/')` on an empty (root) subpath yields a // single "" whose encode is "" — leaving the trailing slash above intact — @@ -206,7 +220,16 @@ pub fn nc_href(username: &str, subpath: &str) -> String { } out.push_str(&urlencoding::encode(seg)); } - out +} + +/// Per-row form of [`nc_collection_href`]: [`nc_href_into`] plus the trailing +/// `/` RFC 4918 §5.2 / the NC client require for a collection. Byte-identical to +/// [`nc_collection_href`]. +pub fn nc_collection_href_into(out: &mut String, encoded_user: &str, subpath: &str) { + nc_href_into(out, encoded_user, subpath); + if !out.ends_with('/') { + out.push('/'); + } } /// Dispatch Nextcloud WebDAV request to the appropriate handler.