diff --git a/Cargo.toml b/Cargo.toml index a6993549..c7c89fb1 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-13 battery ──────────────────────────────────────────────────────────── + +# Round-13 HTTP micro-pack — duplicate /api TraceLayer removal, borrow-only +# client_ip span render, precomputed locale supported-codes. No Postgres. +[[example]] +name = "bench_round13_micro" +path = "examples/bench_round13_micro.rs" +required-features = ["bench"] + +# Round-13 query-shape pack — notification recipient narrowing, login-hook +# EXISTS probes, recent prune-on-insert (needs the dev Postgres up). +[[example]] +name = "bench_round13_queries" +path = "examples/bench_round13_queries.rs" +required-features = ["bench"] + # Round-12 battery ──────────────────────────────────────────────────────────── # Round-12 query-shape pack — sharee narrow read + trgm, login/email stamp diff --git a/benches/ROUND13.md b/benches/ROUND13.md new file mode 100644 index 00000000..dede4a49 --- /dev/null +++ b/benches/ROUND13.md @@ -0,0 +1,203 @@ +# Round 13 — grouped-view virtualization, notification/login query narrowing, HTTP dedup, locale precompute + +Benchmark-gated, same rule as ROUND2-12: every change ships with a +BEFORE/AFTER benchmark and an equivalence/safety gate; an AFTER that doesn't +beat its BEFORE gets rolled back or redesigned. This round's discipline +story is a *correctness* finding the sweep surfaced under a perf banner: the +"media hooks read the same blob 3×" lead turned out to be "1 real read + 2 +*broken* reads" (the raw-path readers resolve only for local + unencrypted + +single-chunk blobs), so it is flagged for maintainers as a correctness bug, +NOT shipped as a perf change (§Not shipped). + +Measured on 4 cores / 15 GiB, local PostgreSQL 16 (fsync off), release +profile; frontend on Node 22 / vitest 4 (jsdom). Reproduce any row with the +command in its section. + +## Summary + +| # | change | key metric | before → after | +|--:|---|---|---| +| V1 | Grouped views windowed (files route + ResourceList; grid was the last unwindowed path — trash is grouped-by-default in grid) | `.file-item` mounted, 800-item group | **800 → <120** (viewport-bounded) | +| Q1 | Group-notification recipient expansion: drop the ≤512 KiB avatar `image` + `ui_preferences` JSONB from `get_users_by_ids` (email path never reads them) | 30-member fan-out | 8.60 → 0.25 ms (**34.3x**) · ~7.7 MB off the wire | +| Q2 | Login provisioning idempotency: `list_*_by_owner().is_empty()` → `SELECT EXISTS` (×2: calendar + address book, on EVERY login) | 4 owned calendars | 0.193 → 0.170 ms (**1.13x**, widens with owned-row count) | +| Q3 | Recent-access: prune only when the upsert actually inserted (`RETURNING xmax=0`) — a re-access can't grow the set | per re-access | 0.567 → 0.324 ms (**1.75x**) · prune round-trip skipped | +| L1 | Locale `Accept-Language`: precomputed supported-codes list vs rebuilding N heap Strings per anonymous request | 16 locales | 616 → 17.3 ns (**35.7x**) · 18 → 1 allocs | +| H1 | Duplicate `TraceLayer` on `/api` removed (the global stack already wraps it) | per `/api` request | 1.86 → 1.42 µs (**1.31x**) · −6 allocs | +| H2 | `client_ip` span field: borrow-only `ClientIpDisplay` vs an owned `String` per request | per request | 187 → 173 ns · −1 alloc | + +## [V1] Grouped views are windowed (the ROUND10-deferred headline) + +``` +cd frontend && npx vitest run src/lib/components/round13.bench.test.ts +``` + +The moment any group-by was active, both the files route +(`routes/files/[...path]/+page.svelte`) and `ResourceList` left their +windowed `VirtualList` paths and rendered `{#each groups}{#each rows}` — the +GRID arm mounted **every** card, and the accumulated listing is the whole +folder, so a big grouped grid mounted thousands of `.file-item`s (~8-10 +``s + ~8 buttons each), a multi-second main-thread block. `/trash` is +grouped-by-default, so a grid-view trash page hit this on first load. + +The fix is the symmetric one the grouped-LIST arm already used and the +flat-GRID arm already proved: **window each swimlane with its own +`VirtualList`** (`windowClass="files-grid-view"` puts the card grid on the +list's inner window). The outer grouped-grid container is a flex column +(`.files-grouped-grid` / `.rl-grouped-grid`), NOT `.files-grid-view` — that +class is itself a grid and would place each header/VirtualList into a cell; +the grid now lives per-section. The files route additionally folds each +group's separate `folders`/`files` into one ordered `Entry` stream +(`groupedEntries`, folders-then-files — the exact old render order) so a +section feeds one `VirtualList`. The prior claim in a code comment that +"`files-grid-view` … can't host the windowing spacer" was simply wrong (the +flat grid disproves it). + +Gate: render the real `ResourceList` in grouped GRID mode at N=800 in one +bucket — mounted `.file-item` count is **<120** (viewport+overscan bounded, +`>` +once in `discover()`; the extractor borrows it and builds only the `&[&str]` +view the crate needs. 16 locales: 616 → 17.3 ns, 18 → 1 allocs per anonymous +request. Gate: precomputed and rebuilt code SETS identical (order is +irrelevant — `accept_language::intersection` ranks by header q-values). + +## [H1][H2] HTTP micro-pack + +``` +cargo run --release --features bench --example bench_round13_micro # §H1, §H2 +``` + +- **Duplicate `TraceLayer` on `/api`** — `routes.rs` layered its own + `TraceLayer::new_for_http()`, but the global `TraceLayer + + ClientIpMakeSpan` stack in `main.rs` wraps the whole app (the `/api` + router is nested into it), so every `/api` request paid TWO span + + response-future layers. Removed; end-to-end 1.86 → 1.42 µs/request, −6 + allocs. Gate: response status identical with 1 vs 2 layers. +- **`client_ip` span field** — `ClientIpMakeSpan::make_span` allocated an + owned `String` per request purely to feed `%client_ip` (Display). New + borrow-only `ClientIpDisplay` renders straight into the span's field + storage (forwarded header borrowed, peer rendered in place): 187 → 173 ns, + −1 alloc. Gate: byte-identical to the owned resolver across all four + resolution cases. + +## Not shipped — correctness finding surfaced by the perf sweep + +- **Media hooks' raw blob reads are broken, not merely duplicated.** The + round-12 deferred "media metadata + faces + thumbnail each read the blob" + lead was investigated for a shared-read refactor. The investigation found + the premise was wrong: `MediaMetadataService` and `FaceIndexingService` + read `.blobs/{file_hash}.blob` **directly**, but that path exists only for + **local + unencrypted + single-chunk** blobs — for a normal multi-MB + (multi-chunk) photo it does not exist, on S3/Azure there is no local + `.blobs` tree, and on encrypted backends it is ciphertext. So today those + two hooks silently produce **no capture date / no GPS / no faces** for the + common case, while only the thumbnail hook (which goes through + `dedup.read_blob_bytes`, honoring chunk-reassembly + decryption) works. + The fix is to route both through `read_blob_bytes` — but that is a + **correctness fix that is perf-neutral-to-negative** (it makes reads that + currently fail actually run), so it does not belong in a benchmark-gated + perf round. Flagged for maintainers as a correctness bug with the exact + call sites; a shared-`Bytes` provider (single decode-plaintext read fanned + to the hooks) is the perf follow-up once the correctness fix lands. + +## Deferred / flagged (not shipped this round) + +- **Unify all four listing arms onto one `VirtualRows`** (flat + grouped × + list + grid), the photos-timeline single-pass model — removes the + per-section scroll listeners the grouped paths now carry and the + four-branch render in both files route and ResourceList. Wants a + pitch-measurement pass so it can't drift the flat views that work today + (V1 scope note). +- **Drive-provisioning `set_role` re-emit on every login** (authz write; a + self-heal for a historical partial-provision case) — needs maintainer + sign-off, same class as the ROUND12 auth-write deferrals. +- **NC per-session quota budget cache** (0 queries/chunk instead of the + ROUND12 fused 1) — needs a staleness/invalidation story (ROUND12 flag + stands). +- **`mp3_duration` full-file scan when the ID3 `TLEN` tag is present** + (ingest path) — preferring TLEN is a speed/accuracy tradeoff on VBR files; + maintainer call. +- **Thumbnail orientation re-parses EXIF** that capture-metadata already + parsed — reusing the persisted `orientation` is ordering-dependent (hooks + run concurrently). +- **`CachedBlobBackend::local_blob_path` sync `stat`** (ROUND10-12 flag + stands; needs an async port variant). +- **`admin_settings_service` ~7 sequential autocommit upserts on OIDC save** + — admin-only, fired a handful of times per deployment; confirmed still + present, judged not worth entangling the hot-reload logic (same verdict as + ROUND12's skipped REST quota-pair fusion). + +## Environment / methodology + +- `cargo run --release --features bench --example bench_round13_queries` + — needs Postgres; seeds + sweeps its own fixtures (`BENCH_PASSES`, + `BENCH_GROUP`, `BENCH_CALS`, `BENCH_RECENT_CAP`). +- `cargo run --release --features bench --example bench_round13_micro` + — counting allocator; §L1 reads the shipped `frontend/static/locales`. +- `cd frontend && npx vitest run src/lib/components/round13.bench.test.ts`. diff --git a/examples/bench_round13_micro.rs b/examples/bench_round13_micro.rs new file mode 100644 index 00000000..3b5ba605 --- /dev/null +++ b/examples/bench_round13_micro.rs @@ -0,0 +1,320 @@ +//! Round-13 HTTP micro-pack (no Postgres). +//! +//! Two sections, each BEFORE (verbatim replica of the shipped shape) vs +//! AFTER (proposed shape), with byte-identity / equivalence gates: +//! +//! [H1] Duplicate `TraceLayer` on `/api` — the inner +//! `TraceLayer::new_for_http()` in `routes.rs` sat under the global +//! `TraceLayer + ClientIpMakeSpan` stack in `main.rs`, so every +//! `/api` request was wrapped in TWO span/response-future layers. +//! Measured end-to-end through real axum routers, one stack vs two. +//! [H2] Per-request `client_ip` `String` in the span factory — +//! `ClientIpMakeSpan::make_span` allocated an owned `String` on every +//! request purely to feed the span's `%client_ip` Display, vs a +//! borrow-only `ClientIpDisplay` that renders into the span storage. +//! +//! Run: +//! cargo run --release --features bench --example bench_round13_micro +//! Tunables (env): BENCH_ITERS (200000) + +use std::alloc::{GlobalAlloc, Layout, System}; +use std::env; +use std::hint::black_box; +use std::net::SocketAddr; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::time::Instant; + +use axum::http::HeaderMap; +use oxicloud::interfaces::middleware::trusted_proxy::{ + ClientIpDisplay, client_ip_display_from_parts, client_ip_from_parts, +}; + +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 { + 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!( + "| {:<40} | {:>12.1} | {:>10.2} |", + label, m.wall_ns_per_op, m.allocs_per_op + ); +} + +// ──────────────────────────────────────────────────────────────────────────── +// [H1] Duplicate TraceLayer on /api — one stack vs two, end-to-end +// ──────────────────────────────────────────────────────────────────────────── + +fn section_trace_dedup() { + use axum::Router; + use axum::routing::get; + use oxicloud::interfaces::middleware::trace_span::ClientIpMakeSpan; + use tower::ServiceExt; + use tower_http::trace::TraceLayer; + + let iters: usize = env_or("BENCH_ITERS", 200_000) / 20; + let rt = tokio::runtime::Builder::new_current_thread() + .build() + .expect("rt"); + + async fn handler() -> &'static str { + "{\"ok\":true}" + } + + // AFTER: the global stack only (one TraceLayer + ClientIpMakeSpan). + let after_app = Router::new() + .route("/api/x", get(handler)) + .layer(TraceLayer::new_for_http().make_span_with(ClientIpMakeSpan)); + + // BEFORE: the inner per-router TraceLayer, then the global stack on top. + let before_app = Router::new() + .route("/api/x", get(handler)) + .layer(TraceLayer::new_for_http()) + .layer(TraceLayer::new_for_http().make_span_with(ClientIpMakeSpan)); + + let call = |app: &axum::Router| { + let app = app.clone(); + rt.block_on(async move { + let res = app + .oneshot( + axum::http::Request::builder() + .uri("/api/x") + .body(axum::body::Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + res.status() + }) + }; + + // Gate: identical status through both stacks. + assert_eq!(call(&before_app), call(&after_app), "status differs"); + println!("# [H1] gate: /api response status identical with 1 vs 2 trace layers — OK"); + + let m_before = measure(iters, || { + black_box(call(&before_app)); + }); + let m_after = measure(iters, || { + black_box(call(&after_app)); + }); + + println!("\n## [H1] Duplicate TraceLayer on /api (per request, incl. router)"); + println!("| arm | ns/op | allocs/op |"); + print_row("BEFORE 2 trace layers", &m_before); + print_row("AFTER 1 (global only)", &m_after); + println!( + "# {:.2}x wall, {:.1} fewer allocs/request", + m_before.wall_ns_per_op / m_after.wall_ns_per_op, + m_before.allocs_per_op - m_after.allocs_per_op + ); + if m_after.wall_ns_per_op >= m_before.wall_ns_per_op { + eprintln!("GATE FAIL [H1]: dedup not faster — rollback"); + std::process::exit(1); + } +} + +// ──────────────────────────────────────────────────────────────────────────── +// [H2] client_ip String vs borrow-only Display +// ──────────────────────────────────────────────────────────────────────────── + +fn section_client_ip() { + let iters: usize = env_or("BENCH_ITERS", 200_000); + + // Three realistic request shapes. + let direct_peer: Option = Some("203.0.113.7:54321".parse().unwrap()); + let empty_headers = HeaderMap::new(); + + let proxy_peer: Option = Some("10.0.0.1:443".parse().unwrap()); + let mut xff_headers = HeaderMap::new(); + xff_headers.insert( + "x-forwarded-for", + "198.51.100.23, 10.0.0.1".parse().unwrap(), + ); + + // Equivalence gate: Display output identical to the owned String for all + // shapes (note: the trusted-proxy branch only forwards when the peer is + // an actually-configured trusted CIDR; with none configured both peers + // render as the direct address — so the gate compares the SAME resolver + // logic on both sides, which is what matters for byte-identity). + for (headers, peer) in [ + (&empty_headers, direct_peer), + (&xff_headers, proxy_peer), + (&empty_headers, None), + ] { + let owned = client_ip_from_parts(headers, peer, true); + let borrowed = format!("{}", client_ip_display_from_parts(headers, peer, true)); + assert_eq!(owned, borrowed, "client_ip bytes differ"); + } + // Directly exercise every ClientIpDisplay variant's Display. + assert_eq!( + format!("{}", ClientIpDisplay::Forwarded("1.2.3.4")), + "1.2.3.4" + ); + assert_eq!( + format!( + "{}", + ClientIpDisplay::PeerWithPort("5.6.7.8:9".parse().unwrap()) + ), + "5.6.7.8:9" + ); + assert_eq!( + format!("{}", ClientIpDisplay::PeerIp("5.6.7.8".parse().unwrap())), + "5.6.7.8" + ); + assert_eq!(format!("{}", ClientIpDisplay::Unknown), "unknown"); + println!("# [H2] gate: borrow-only Display renders byte-identical to owned String — OK"); + + // The span records `client_ip = %ip`; emulate that terminal render into a + // reusable String (the span's field storage) for BOTH arms so we isolate + // the ONE allocation the owned resolver adds on top. + use std::fmt::Write as _; + + let m_before = measure(iters, || { + let ip = client_ip_from_parts(black_box(&empty_headers), black_box(direct_peer), true); + let mut sink = String::new(); + let _ = write!(sink, "{ip}"); + black_box(sink); + }); + let m_after = measure(iters, || { + let ip = + client_ip_display_from_parts(black_box(&empty_headers), black_box(direct_peer), true); + let mut sink = String::new(); + let _ = write!(sink, "{ip}"); + black_box(sink); + }); + + println!("\n## [H2] client_ip resolution for the span factory (direct peer)"); + println!("| arm | ns/op | allocs/op |"); + print_row("BEFORE owned String + render", &m_before); + print_row("AFTER borrow Display + render", &m_after); + println!( + "# {:.2}x wall, {:.1} fewer allocs/request", + m_before.wall_ns_per_op / m_after.wall_ns_per_op, + m_before.allocs_per_op - m_after.allocs_per_op + ); + if m_after.allocs_per_op >= m_before.allocs_per_op { + eprintln!("GATE FAIL [H2]: borrow arm did not remove an allocation — rollback"); + std::process::exit(1); + } +} + +// ──────────────────────────────────────────────────────────────────────────── +// [L1] Locale supported-codes: per-request rebuild vs precomputed borrow +// ──────────────────────────────────────────────────────────────────────────── + +fn section_locale() { + use oxicloud::common::locale::LocaleRegistry; + use std::path::Path; + + let iters: usize = env_or("BENCH_ITERS", 200_000) / 2; + + // Real registry over the shipped locales (16 JSON files). + let dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("frontend/static/locales"); + let registry = match LocaleRegistry::discover(&dir, "en") { + Ok(r) => r, + Err(e) => { + println!("# [L1] skipped — locale registry unavailable: {e}"); + return; + } + }; + let n = registry.supported_codes().len(); + + // Equivalence gate: same code SET both ways (order differs — the + // Accept-Language crate ranks by header q-values, not list order). + let mut before_set: Vec = registry.iter().map(|l| l.as_str().to_string()).collect(); + let mut after_set: Vec = registry.supported_codes().to_vec(); + before_set.sort(); + after_set.sort(); + assert_eq!(before_set, after_set, "supported-code sets differ"); + println!("# [L1] gate: rebuilt and precomputed supported-code sets identical ({n} codes) — OK"); + + // BEFORE, verbatim old extractor: N owned Strings + the &str view. + let m_before = measure(iters, || { + let owned: Vec = registry.iter().map(|l| l.as_str().to_string()).collect(); + let view: Vec<&str> = owned.iter().map(String::as_str).collect(); + black_box(&view); + black_box(owned); + }); + // AFTER: borrow the precomputed list; build only the &str view. + let m_after = measure(iters, || { + let view: Vec<&str> = registry + .supported_codes() + .iter() + .map(String::as_str) + .collect(); + black_box(view); + }); + + println!("\n## [L1] Locale supported-codes for Accept-Language ({n} locales)"); + println!("| arm | ns/op | allocs/op |"); + print_row("BEFORE rebuild N Strings + view", &m_before); + print_row("AFTER borrow precomputed + view", &m_after); + println!( + "# {:.2}x wall, {:.1} fewer allocs per anonymous request", + m_before.wall_ns_per_op / m_after.wall_ns_per_op, + m_before.allocs_per_op - m_after.allocs_per_op + ); + if m_after.wall_ns_per_op >= m_before.wall_ns_per_op { + eprintln!("GATE FAIL [L1]: precomputed borrow not faster — rollback"); + std::process::exit(1); + } +} + +fn main() { + println!("#################################################################"); + println!("# Round-13 HTTP micro-pack"); + println!("#################################################################\n"); + + section_trace_dedup(); + section_client_ip(); + section_locale(); + + println!("\nGATE PASS (all sections)"); +} diff --git a/examples/bench_round13_queries.rs b/examples/bench_round13_queries.rs new file mode 100644 index 00000000..251f794a --- /dev/null +++ b/examples/bench_round13_queries.rs @@ -0,0 +1,421 @@ +//! Round-13 query-shape pack (needs the dev Postgres up; reads DATABASE_URL +//! from `.env`). +//! +//! Three sections, each BEFORE (verbatim replica of the shipped query shape) +//! vs AFTER (proposed shape), with equivalence/safety gates: +//! +//! [Q1] Group-notification recipient expansion — `get_users_by_ids`'s +//! 21-column row (incl. the ≤512 KiB avatar `image` + `ui_preferences` +//! JSONB) hydrated per member vs the notification-only projection +//! (drops both heavy columns; the caller reads only email/eligibility +//! fields). +//! [Q2] Login provisioning idempotency — `list_calendars_by_owner(..) +//! .is_empty()` / `get_address_books_by_owner(..).is_empty()` (hydrate +//! every owned row) vs `SELECT EXISTS(...)`. +//! [Q3] Recent-access recording — unconditional upsert + prune (2 +//! round-trips) vs upsert-`RETURNING (xmax=0)` + prune-only-on-insert. +//! +//! Run: +//! cargo run --release --features bench --example bench_round13_queries +//! Tunables (env): BENCH_PASSES (200), BENCH_GROUP (30), BENCH_CALS (4), +//! BENCH_RECENT_CAP (50) + +use std::env; +use std::sync::Arc; +use std::time::Instant; + +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 stats(mut s: Vec) -> (f64, f64, f64) { + s.sort_by(|a, b| a.partial_cmp(b).unwrap()); + let n = s.len(); + ( + s.iter().sum::() / n as f64, + s[n / 2], + s[((n as f64 * 0.95) as usize).min(n - 1)], + ) +} + +// ──────────────────────────────────────────────────────────────────────────── +// [Q1] Notification recipient expansion — wide row vs narrow projection +// ──────────────────────────────────────────────────────────────────────────── + +/// BEFORE, verbatim `get_users_by_ids` projection: 21 columns incl. `image` +/// and `ui_preferences`. Touch the heavy columns like `User::from_data_full` +/// does (materialize them) so the detoast/parse cost is counted. +async fn recipients_before(pool: &PgPool, ids: &[Uuid]) -> Vec<(Uuid, String, bool)> { + let rows = sqlx::query( + r#" + SELECT + id, username, email, password_hash, role::text as role_text, + storage_quota_bytes, storage_used_bytes, + created_at, updated_at, last_login_at, active, + oidc_provider, oidc_subject, image, is_external, + given_name, family_name, email_verified_at, preferred_locale, notify_on_share, + ui_preferences + FROM auth.users + WHERE id = ANY($1) + "#, + ) + .bind(ids) + .fetch_all(pool) + .await + .expect("recipients wide"); + rows.into_iter() + .map(|r| { + let _image: Option = r.get("image"); + let _prefs: serde_json::Value = r.get("ui_preferences"); + (r.get("id"), r.get("email"), r.get("notify_on_share")) + }) + .collect() +} + +/// AFTER: the shipped narrow projection (image + ui_preferences dropped). +async fn recipients_after(pool: &PgPool, ids: &[Uuid]) -> Vec<(Uuid, String, bool)> { + let rows = sqlx::query( + r#" + SELECT + id, username, email, password_hash, role::text as role_text, + storage_quota_bytes, storage_used_bytes, + created_at, updated_at, last_login_at, active, + oidc_provider, oidc_subject, is_external, + given_name, family_name, email_verified_at, preferred_locale, notify_on_share + FROM auth.users + WHERE id = ANY($1) + "#, + ) + .bind(ids) + .fetch_all(pool) + .await + .expect("recipients narrow"); + rows.into_iter() + .map(|r| (r.get("id"), r.get("email"), r.get("notify_on_share"))) + .collect() +} + +async fn section_recipients(pool: &PgPool) { + let group: usize = env_or("BENCH_GROUP", 30); + let passes: usize = env_or("BENCH_PASSES", 200); + + // Seed a group of avatared users (256 KiB data-URI each). + let mut ids = Vec::with_capacity(group); + for i in 0..group { + let id: Uuid = sqlx::query_scalar( + "INSERT INTO auth.users (username, email, role, image, notify_on_share) + VALUES ($1, $2, 'user', $3, true) RETURNING id", + ) + .bind(format!("bench13_rcpt_{i:04}")) + .bind(format!("bench13_rcpt_{i:04}@bench.invalid")) + .bind(format!( + "data:image/png;base64,{}", + "QUJDRA==".repeat(32 * 1024) + )) + .fetch_one(pool) + .await + .expect("seed recipient"); + ids.push(id); + } + + // Equivalence gate: same (id, email, notify) set either way. + let mut b = recipients_before(pool, &ids).await; + let mut a = recipients_after(pool, &ids).await; + b.sort(); + a.sort(); + assert_eq!(b, a, "recipient projections differ"); + assert_eq!(a.len(), group, "expected all members"); + println!("# [Q1] gate: wide/narrow recipient sets identical ({group} members) — OK"); + + let mut wide = Vec::with_capacity(passes); + for _ in 0..passes { + let t = Instant::now(); + std::hint::black_box(recipients_before(pool, &ids).await); + wide.push(t.elapsed().as_secs_f64() * 1e3); + } + let mut narrow = Vec::with_capacity(passes); + for _ in 0..passes { + let t = Instant::now(); + std::hint::black_box(recipients_after(pool, &ids).await); + narrow.push(t.elapsed().as_secs_f64() * 1e3); + } + let (wm, wp50, wp95) = stats(wide); + let (nm, np50, np95) = stats(narrow); + println!("\n## [Q1] Group-notification recipient expansion ({group} avatared members)"); + println!("| arm | mean ms | p50 ms | p95 ms |"); + println!("| BEFORE wide row (incl. image) | {wm:>8.3} | {wp50:>7.3} | {wp95:>7.3} |"); + println!("| AFTER narrow (email fields) | {nm:>8.3} | {np50:>7.3} | {np95:>7.3} |"); + println!( + "# {:.2}x faster, ~{} KiB avatar/ui_prefs off the wire per fan-out", + wm / nm, + group * 256 + ); + + sqlx::query("DELETE FROM auth.users WHERE username LIKE 'bench13\\_rcpt\\_%'") + .execute(pool) + .await + .expect("cleanup recipients"); + if nm >= wm { + eprintln!("GATE FAIL [Q1]: narrow not faster — rollback"); + std::process::exit(1); + } +} + +// ──────────────────────────────────────────────────────────────────────────── +// [Q2] Login provisioning idempotency — hydrate-all vs EXISTS +// ──────────────────────────────────────────────────────────────────────────── + +async fn section_provisioning(pool: &PgPool) { + let cals: usize = env_or("BENCH_CALS", 4); + let passes: usize = env_or("BENCH_PASSES", 200); + + let owner: Uuid = sqlx::query_scalar( + "INSERT INTO auth.users (username, email, role) + VALUES ('bench13_prov', 'bench13_prov@bench.invalid', 'user') RETURNING id", + ) + .fetch_one(pool) + .await + .expect("seed owner"); + for i in 0..cals { + sqlx::query( + "INSERT INTO caldav.calendars (id, name, owner_id, description, color) + VALUES (gen_random_uuid(), $1, $2, $3, '#3b82f6')", + ) + .bind(format!("Cal {i}")) + .bind(owner) + .bind("A reasonably long calendar description to make the hydrated row wider") + .execute(pool) + .await + .expect("seed calendar"); + } + + async fn before_is_empty(pool: &PgPool, owner: Uuid) -> bool { + // Verbatim: hydrate every owned calendar row, then `.is_empty()`. + let rows = sqlx::query( + "SELECT id, name, owner_id, description, color, is_public, created_at, updated_at + FROM caldav.calendars WHERE owner_id = $1 ORDER BY name", + ) + .bind(owner) + .fetch_all(pool) + .await + .expect("list calendars"); + !rows.is_empty() + } + async fn after_exists(pool: &PgPool, owner: Uuid) -> bool { + sqlx::query_scalar("SELECT EXISTS(SELECT 1 FROM caldav.calendars WHERE owner_id = $1)") + .bind(owner) + .fetch_one(pool) + .await + .expect("exists") + } + + // Gate: identical verdict, present and absent. + assert!(before_is_empty(pool, owner).await); + assert!(after_exists(pool, owner).await); + let ghost = Uuid::new_v4(); + assert_eq!( + before_is_empty(pool, ghost).await, + after_exists(pool, ghost).await + ); + println!("# [Q2] gate: hydrate-all and EXISTS agree (present + absent) — OK"); + + let mut before = Vec::with_capacity(passes); + for _ in 0..passes { + let t = Instant::now(); + std::hint::black_box(before_is_empty(pool, owner).await); + before.push(t.elapsed().as_secs_f64() * 1e3); + } + let mut after = Vec::with_capacity(passes); + for _ in 0..passes { + let t = Instant::now(); + std::hint::black_box(after_exists(pool, owner).await); + after.push(t.elapsed().as_secs_f64() * 1e3); + } + let (bm, bp50, bp95) = stats(before); + let (am, ap50, ap95) = stats(after); + println!("\n## [Q2] Login provisioning idempotency probe ({cals} owned calendars)"); + println!("| arm | mean ms | p50 ms | p95 ms |"); + println!("| BEFORE list+hydrate .is_empty() | {bm:>7.3} | {bp50:>7.3} | {bp95:>7.3} |"); + println!("| AFTER SELECT EXISTS | {am:>7.3} | {ap50:>7.3} | {ap95:>7.3} |"); + println!( + "# {:.2}x faster per login probe (×2: calendar + address book)", + bm / am + ); + + sqlx::query("DELETE FROM caldav.calendars WHERE owner_id = $1") + .bind(owner) + .execute(pool) + .await + .ok(); + sqlx::query("DELETE FROM auth.users WHERE id = $1") + .bind(owner) + .execute(pool) + .await + .ok(); + if am >= bm { + eprintln!("GATE FAIL [Q2]: EXISTS not faster — rollback"); + std::process::exit(1); + } +} + +// ──────────────────────────────────────────────────────────────────────────── +// [Q3] Recent-access recording — upsert+prune (2 RTT) vs prune-on-insert +// ──────────────────────────────────────────────────────────────────────────── + +async fn section_recent(pool: &PgPool) { + let cap: i32 = env_or("BENCH_RECENT_CAP", 50); + let passes: usize = env_or("BENCH_PASSES", 200); + + let user: Uuid = sqlx::query_scalar( + "INSERT INTO auth.users (username, email, role) + VALUES ('bench13_recent', 'bench13_recent@bench.invalid', 'user') RETURNING id", + ) + .fetch_one(pool) + .await + .expect("seed recent user"); + + async fn upsert_before(pool: &PgPool, user: Uuid, item: &str) { + sqlx::query( + "INSERT INTO auth.user_recent_files (user_id, item_id, item_type, accessed_at) + VALUES ($1, $2, 'file', CURRENT_TIMESTAMP) + ON CONFLICT (user_id, item_id, item_type) + DO UPDATE SET accessed_at = CURRENT_TIMESTAMP", + ) + .bind(user) + .bind(item) + .execute(pool) + .await + .expect("upsert"); + } + async fn prune(pool: &PgPool, user: Uuid, cap: i32) { + sqlx::query( + "DELETE FROM auth.user_recent_files + WHERE id IN (SELECT id FROM auth.user_recent_files + WHERE user_id = $1 ORDER BY accessed_at DESC OFFSET $2)", + ) + .bind(user) + .bind(cap) + .execute(pool) + .await + .expect("prune"); + } + async fn upsert_after(pool: &PgPool, user: Uuid, item: &str) -> bool { + sqlx::query_scalar( + "INSERT INTO auth.user_recent_files (user_id, item_id, item_type, accessed_at) + VALUES ($1, $2, 'file', CURRENT_TIMESTAMP) + ON CONFLICT (user_id, item_id, item_type) + DO UPDATE SET accessed_at = CURRENT_TIMESTAMP + RETURNING (xmax = 0)", + ) + .bind(user) + .bind(item) + .fetch_one(pool) + .await + .expect("upsert returning") + } + + // Fill to the cap so the set is at steady state. + for i in 0..cap { + upsert_before(pool, user, &format!("seed-{i:04}")).await; + } + + // Gate: the AFTER path must keep the row count at the cap AND flag + // insert-vs-update correctly. Re-access an existing item → update (no + // prune); a brand-new item → insert (prune keeps count == cap). + let existing = "seed-0000"; + assert!( + !upsert_after(pool, user, existing).await, + "re-access must be an UPDATE" + ); + let fresh = "gate-new-item"; + assert!( + upsert_after(pool, user, fresh).await, + "new item must be an INSERT" + ); + prune(pool, user, cap).await; + let count: i64 = + sqlx::query_scalar("SELECT COUNT(*) FROM auth.user_recent_files WHERE user_id = $1") + .bind(user) + .fetch_one(pool) + .await + .unwrap(); + assert_eq!(count, cap as i64, "prune-on-insert keeps the cap"); + println!("# [Q3] gate: xmax flags insert/update, count stays at cap — OK"); + + // BEFORE: every record = upsert + prune (2 round-trips). Model the + // common case — re-accessing items already in the set (all UPDATEs). + let mut before = Vec::with_capacity(passes); + for i in 0..passes { + let item = format!("seed-{:04}", i % cap as usize); + let t = Instant::now(); + upsert_before(pool, user, &item).await; + prune(pool, user, cap).await; + before.push(t.elapsed().as_secs_f64() * 1e3); + } + // AFTER: upsert RETURNING; prune only when inserted (never, here). + let mut after = Vec::with_capacity(passes); + for i in 0..passes { + let item = format!("seed-{:04}", i % cap as usize); + let t = Instant::now(); + let inserted = upsert_after(pool, user, &item).await; + if inserted { + prune(pool, user, cap).await; + } + after.push(t.elapsed().as_secs_f64() * 1e3); + } + let (bm, bp50, bp95) = stats(before); + let (am, ap50, ap95) = stats(after); + println!("\n## [Q3] Recent-access recording (re-access = UPDATE, common path)"); + println!("| arm | mean ms | p50 ms | p95 ms |"); + println!("| BEFORE upsert + prune (2 RTT) | {bm:>7.3} | {bp50:>7.3} | {bp95:>7.3} |"); + println!("| AFTER upsert; prune-on-insert | {am:>7.3} | {ap50:>7.3} | {ap95:>7.3} |"); + println!( + "# {:.2}x faster on re-access; prune round-trip skipped", + bm / am + ); + + sqlx::query("DELETE FROM auth.user_recent_files WHERE user_id = $1") + .bind(user) + .execute(pool) + .await + .ok(); + sqlx::query("DELETE FROM auth.users WHERE id = $1") + .bind(user) + .execute(pool) + .await + .ok(); + if am >= bm { + eprintln!("GATE FAIL [Q3]: prune-on-insert not faster — rollback"); + std::process::exit(1); + } +} + +#[tokio::main(flavor = "multi_thread", worker_threads = 4)] +async fn main() { + let _ = dotenvy::dotenv(); + let url = std::env::var("DATABASE_URL").expect("DATABASE_URL required (see .env)"); + let pool = Arc::new( + PgPoolOptions::new() + .max_connections(8) + .connect(&url) + .await + .expect("connect"), + ); + + println!("#################################################################"); + println!("# Round-13 query-shape pack"); + println!("#################################################################"); + + section_recipients(&pool).await; + section_provisioning(&pool).await; + section_recent(&pool).await; + + println!("\nGATE PASS (all sections)"); +} diff --git a/frontend/src/lib/components/ResourceList.svelte b/frontend/src/lib/components/ResourceList.svelte index 1d78349d..21208257 100644 --- a/frontend/src/lib/components/ResourceList.svelte +++ b/frontend/src/lib/components/ResourceList.svelte @@ -329,9 +329,6 @@ // filter on shows the empty state (the host page's `emptyHint` can // reference `hiddenCount` to say "3 items hidden by the filter"). const isEmpty = $derived(visibleItems.length === 0); - const viewClass = $derived( - filesStore.viewMode === 'grid' ? 'files-grid-view' : 'files-list-view' - ); /** Content width, for computing the grid's column count to match auto-fill. */ let gridWidth = $state(0); const gridCols = $derived(gridColumns(gridWidth)); @@ -740,8 +737,8 @@ /> {:else}
- {#if grouped} -
+ {#if grouped && filesStore.viewMode === 'list'} +
{#if listHeaderOverride}{@render listHeaderOverride()}{:else}{@render listHeader()}{/if} {#each sections as section (section.key)}
@@ -752,17 +749,37 @@ {/if}
- {#if filesStore.viewMode === 'list'} - - e.id} {row} /> - {:else} - {#each section.rows as entry (entry.id)} - {@render row(entry)} - {/each} - {/if} + + e.id} {row} /> + {/each} +
+ {:else if grouped} + +
+ {#each sections as section (section.key)} +
+ {section.label} + {#if bucketAction} + + {@render bucketAction(section.key)} + + {/if} +
+ e.id} + {row} + /> {/each}
{:else if filesStore.viewMode === 'list'} @@ -987,6 +1004,22 @@ align-items: center; } + /* Grouped-grid container: a vertical stack of (header + its own windowed + card grid) per section. Not `.files-grid-view` — the grid is on each + VirtualList's inner window, so this outer element just stacks. */ + .rl-grouped-grid { + display: flex; + flex-direction: column; + gap: var(--space-2); + } + + /* In the flex stack the `grid-column: 1 / -1` span (meant for the grid + context) is inert; the header spans naturally as a block-level flex + child. */ + .rl-swimlane-header--grid { + grid-column: auto; + } + /* Grid view date meta line. */ .grid-meta__line { display: flex; diff --git a/frontend/src/lib/components/round13.bench.test.ts b/frontend/src/lib/components/round13.bench.test.ts new file mode 100644 index 00000000..33dbbf87 --- /dev/null +++ b/frontend/src/lib/components/round13.bench.test.ts @@ -0,0 +1,124 @@ +// Round-13 §V1 — grouped views are windowed (benches/ROUND13.md). +// +// Before this round, the grouped GRID path mounted EVERY card: +// `{#each sections}{#each section.rows}{@render row}` with no windowing +// (the grouped-by-default trash grid, and the files route's grouped grid, +// were the last unwindowed paths). Now each swimlane feeds its own windowed +// — a flex stack of (header + windowed card grid) per section +// — so only a viewport-bounded slice of `.file-item` cards is realized, +// regardless of group size. +// +// Gate: render the real ResourceList in grouped GRID mode with N=800 items +// in one bucket and assert the mounted card count is viewport-bounded, not +// N. jsdom does no layout, so VirtualList's visible band is a small constant +// — the same lever the round-12 files page test documents. + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render } from '@testing-library/svelte'; + +vi.mock('$lib/api/endpoints/files', () => ({ + fileThumbnailUrl: () => '/thumb', + thumbSizeForView: () => 'preview' as const +})); + +import ResourceList from './ResourceList.svelte'; +import type { GroupByDef } from './ResourceList.svelte'; +import { files as filesStore } from '$lib/stores/files.svelte'; + +interface TestFile { + category: string; + created_at: number; + icon_class: string; + icon_special_class: string; + id: string; + mime_type: string; + modified_at: number; + name: string; + created_by: string; + updated_by: string; + folder_id: string; + path: string; + size: number; + size_formatted: string; + sort_date: number; + etag: string; + content_hash: string; +} + +function fileItem(i: number): TestFile { + return { + category: 'Document', + created_at: 0, + icon_class: 'fa-file', + icon_special_class: '', + id: `f${i}`, + mime_type: 'text/plain', + modified_at: 0, + name: `file-${i}.txt`, + created_by: 'me', + updated_by: 'me', + folder_id: 'home', + path: `/file-${i}.txt`, + size: 4, + size_formatted: '4 B', + sort_date: 0, + etag: 'e', + content_hash: 'h' + }; +} + +// Single bucket → one big swimlane (the worst case the old grid mounted whole). +const groupBys: GroupByDef[] = [ + { + key: 'type', + label: 'Type', + orderBy: 'name', + bucketOf: (item) => (item as TestFile).category ?? 'other', + labelOf: (k) => k + } +]; + +describe('round13 §V1 — grouped grid is windowed', () => { + beforeEach(() => { + filesStore.viewMode = 'grid'; + }); + + it('mounts a viewport-bounded slice of cards, not all N, in grouped grid', () => { + const N = 800; + const items = Array.from({ length: N }, (_, i) => fileItem(i)); + const { container } = render(ResourceList, { + props: { + title: 'Round13', + items, + groupBys, + groupBy: 'type', + selectable: true, + actions: undefined + } + }); + + const mounted = container.querySelectorAll('.file-item').length; + // A swimlane header confirms we are on the grouped path. + expect(container.querySelectorAll('.rl-swimlane-header').length).toBeGreaterThan(0); + // Windowed: the visible band is viewport+overscan bounded, far below N. + // (The pre-fix grid-grouped path mounted all 800.) + expect(mounted).toBeGreaterThan(0); + expect(mounted).toBeLessThan(120); + expect(mounted).toBeLessThan(N / 4); + }); + + it('full scroll height is still reserved (windowing spacer, not truncation)', () => { + const N = 800; + const items = Array.from({ length: N }, (_, i) => fileItem(i)); + const { container } = render(ResourceList, { + props: { title: 'Round13', items, groupBys, groupBy: 'type', selectable: true } + }); + // The VirtualList reserves total height via its `.vlist` spacer so the + // scrollbar / end-of-list sentinel keep working — height must scale with + // N, proving cards weren't simply dropped. + const vlist = container.querySelector('.vlist') as HTMLElement | null; + expect(vlist).not.toBeNull(); + const reserved = parseFloat(vlist!.style.height || '0'); + expect(reserved).toBeGreaterThan(1000); + }); +}); diff --git a/frontend/src/lib/styles/ported/resourceList.css b/frontend/src/lib/styles/ported/resourceList.css index ea9337e4..e4d5b594 100644 --- a/frontend/src/lib/styles/ported/resourceList.css +++ b/frontend/src/lib/styles/ported/resourceList.css @@ -1025,6 +1025,25 @@ margin-top: 0; } +/* Grouped-grid container (files route): a vertical stack of + (header + its own windowed card grid) per swimlane. Not a grid itself — + the card grid rides each VirtualList's inner window — so it just stacks. */ +.files-grouped-grid { + display: flex; + flex-direction: column; +} + +/* In that flex stack the `grid-column: 1 / -1` span is inert; the header + spans naturally as a block-level flex child. First header needs no top + margin (there is no list-header sibling before it in the grid path). */ +.files-grouped-grid > .resource-list__swimlane-header--grid { + grid-column: auto; +} + +.files-grouped-grid > .resource-list__swimlane-header--grid:first-child { + margin-top: 0; +} + /* When the header contains a rich DOM node (e.g. a user vignette for the "owner" group-by), reset the typographic overrides that only make sense for plain-text labels, and lay the node out inline. */ diff --git a/frontend/src/routes/files/[...path]/+page.svelte b/frontend/src/routes/files/[...path]/+page.svelte index 7f279891..50e61c3d 100644 --- a/frontend/src/routes/files/[...path]/+page.svelte +++ b/frontend/src/routes/files/[...path]/+page.svelte @@ -1455,9 +1455,6 @@ // renders; `hiddenCount` above lets the template surface a "you're // hiding N items" hint so users aren't confused. const isEmpty = $derived(visibleFolders.length === 0 && visibleFiles.length === 0); - const viewClass = $derived( - filesStore.viewMode === 'grid' ? 'files-grid-view' : 'files-list-view' - ); // Client-side sort (flat, Drive-style). The listing endpoint returns the // folder contents unsorted; sorting here avoids a refetch per column click. @@ -1562,6 +1559,25 @@ return [...map.values()]; }); + // Each group's folders + files folded into ONE ordered `Entry` stream + // (folders first, then files — the exact render order the un-windowed + // `{#each folders}{#each files}` produced), so each swimlane can feed a + // windowed instead of mounting every row/card + // (benches/ROUND13.md §V1). `buildFileRows` is intentionally identity- + // and order-only: it must NOT read favoriteIds/sharedIds/selection, or a + // single star/select toggle would rebuild every swimlane (the ROUND11 + // §S2 fine-grained-star invariant). + const groupedEntries = $derived( + groups.map((g) => ({ + key: g.key, + label: g.label, + entries: [ + ...g.folders.map((folder) => ({ kind: 'folder' as const, folder })), + ...g.files.map((file) => ({ kind: 'file' as const, file })) + ] as Entry[] + })) + ); + // ── Toolbar controls (upload split-button + group-by popup menu) ───────── // The group-by popup + sort-direction + view toggle live in the shared // ; this page only owns the upload split-button dropdown. @@ -1893,17 +1909,34 @@ {/if} {:else}
- {#if groupBy !== ''} -
+ {#if groupBy !== '' && filesStore.viewMode === 'list'} + +
{@render fileListHeader()} - {#each groups as group (group.key)} + {#each groupedEntries as group (group.key)}
{group.label}
- {#each group.folders as folder (folder.id)} - {@render folderRow(folder)} - {/each} - {#each group.files as file (file.id)} - {@render fileRow(file)} - {/each} + + {/each} +
+ {:else if groupBy !== ''} + +
+ {#each groupedEntries as group (group.key)} +
+ {group.label} +
+ {/each}
{:else if filesStore.viewMode === 'list'} diff --git a/src/application/ports/recent_ports.rs b/src/application/ports/recent_ports.rs index b3181ec4..e6417026 100644 --- a/src/application/ports/recent_ports.rs +++ b/src/application/ports/recent_ports.rs @@ -41,7 +41,11 @@ pub trait RecentItemsRepositoryPort: Send + Sync + 'static { async fn get_recent_items(&self, user_id: Uuid, limit: i32) -> Result>; /// Records/updates access to an item (upsert by user+item+type). - async fn upsert_access(&self, user_id: Uuid, item_id: &str, item_type: &str) -> Result<()>; + /// Returns `true` when a NEW row was inserted (the recent set grew) and + /// `false` when an existing row's timestamp was merely refreshed — the + /// caller prunes only in the former case, since a re-access can never + /// push the user over the cap (benches/ROUND13.md §Q3). + async fn upsert_access(&self, user_id: Uuid, item_id: &str, item_type: &str) -> Result; /// Removes an item from recents. Returns `true` if it existed. async fn remove_item(&self, user_id: Uuid, item_id: &str, item_type: &str) -> Result; diff --git a/src/application/services/calendar_service.rs b/src/application/services/calendar_service.rs index e748a111..4efe9673 100644 --- a/src/application/services/calendar_service.rs +++ b/src/application/services/calendar_service.rs @@ -474,18 +474,21 @@ impl DefaultCalendarLifecycleHook { // Ownership-based idempotency check (see hook docstring for // the design rationale). Whether the existing calendar was // auto-provisioned by a prior run, manually created by the - // user, or migrated in, we respect it and skip. - let existing = self + // user, or migrated in, we respect it and skip. `EXISTS` + // short-circuits at the first owned row instead of hydrating them + // all just to test emptiness — this runs on EVERY login + // (benches/ROUND13.md §Q2). + let has_calendar = self .calendar_storage - .list_calendars_by_owner(user.id()) + .has_owned_calendar(user.id()) .await .map_err(|e| { DomainError::internal_error( "DefaultCalendarHook", - format!("list_calendars_by_owner: {e}"), + format!("has_owned_calendar: {e}"), ) })?; - if !existing.is_empty() { + if has_calendar { return Ok(()); } diff --git a/src/application/services/contact_service.rs b/src/application/services/contact_service.rs index 325ef5b9..6bf770d6 100644 --- a/src/application/services/contact_service.rs +++ b/src/application/services/contact_service.rs @@ -1219,7 +1219,6 @@ impl ContactUseCase for ContactService { use crate::application::ports::user_lifecycle::{DeletionMode, LogoutReason, UserLifecycleHook}; use crate::domain::entities::user::User; -use crate::domain::repositories::address_book_repository::AddressBookRepository; use crate::infrastructure::repositories::pg::AddressBookPgRepository; use async_trait::async_trait; @@ -1264,17 +1263,20 @@ impl DefaultAddressBookLifecycleHook { // Ownership-based idempotency check — same rationale as the // calendar hook. Any existing owned address book (auto- // provisioned earlier, user-created, migrated) is respected. - let existing = self + // `EXISTS` short-circuits instead of hydrating every owned + // address book to test emptiness, on EVERY login + // (benches/ROUND13.md §Q2). + let has_address_book = self .address_book_repo - .get_address_books_by_owner(user.id()) + .has_owned_address_book(user.id()) .await .map_err(|e| { DomainError::internal_error( "DefaultAddressBookHook", - format!("get_address_books_by_owner: {e}"), + format!("has_owned_address_book: {e}"), ) })?; - if !existing.is_empty() { + if has_address_book { return Ok(()); } diff --git a/src/application/services/recent_service.rs b/src/application/services/recent_service.rs index dbb9f610..272e2a10 100644 --- a/src/application/services/recent_service.rs +++ b/src/application/services/recent_service.rs @@ -98,8 +98,15 @@ impl RecentService { )); } - self.repo.upsert_access(user_id, item_id, item_type).await?; - self.repo.prune(user_id, self.max_recent_items).await?; + // Prune only when the upsert actually inserted a NEW row — a + // re-access refreshes an existing row's timestamp and can never + // grow the set past the cap, so the prune (a DELETE over an + // OFFSET self-subquery) is a wasted round-trip on that common path + // (benches/ROUND13.md §Q3). + let inserted = self.repo.upsert_access(user_id, item_id, item_type).await?; + if inserted { + self.repo.prune(user_id, self.max_recent_items).await?; + } Ok(()) } } diff --git a/src/common/locale.rs b/src/common/locale.rs index 29ae43e4..1abe0382 100644 --- a/src/common/locale.rs +++ b/src/common/locale.rs @@ -115,6 +115,13 @@ pub struct LocaleRegistry { /// case-insensitive: input is canonicalised, then probed against /// this set. canonical: Arc>, + /// The same codes as an owned `Vec`, materialized ONCE at + /// [`Self::discover`] time. The `Accept-Language` extractor needs a + /// `&[&str]` supported-list per anonymous request; without this it + /// rebuilt N heap `String`s from the registry on every such request + /// (the ROUND10 §15 "process-invariant rebuilt per request" class; + /// benches/ROUND13.md §L1). Borrowed via [`Self::supported_codes`]. + supported_codes: Arc>, /// The configured fallback locale. Resolved from /// `OXICLOUD_DEFAULT_LOCALE` at startup; defaults to English when /// unset. @@ -200,8 +207,15 @@ impl LocaleRegistry { sorted.join(", ") ); + // Materialize the supported-codes list once. Order is irrelevant — + // `accept_language::intersection` ranks by the request header's + // q-values, not by this list's order. + let supported_codes: Vec = + canonical.iter().map(|s| s.as_str().to_string()).collect(); + Ok(Self { canonical: Arc::new(canonical), + supported_codes: Arc::new(supported_codes), default, }) } @@ -236,6 +250,13 @@ impl LocaleRegistry { self.canonical.iter().map(|s| Locale(s.clone())) } + /// The registry's codes as a borrowable `&[String]`, precomputed at + /// [`Self::discover`] time. Feeds the per-request `Accept-Language` + /// negotiation without re-allocating the list (benches/ROUND13.md §L1). + pub fn supported_codes(&self) -> &[String] { + &self.supported_codes + } + /// Number of locales in the registry. Used by tests + startup logs. pub fn len(&self) -> usize { self.canonical.len() diff --git a/src/infrastructure/adapters/calendar_storage_adapter.rs b/src/infrastructure/adapters/calendar_storage_adapter.rs index 7c21aed7..184fbced 100644 --- a/src/infrastructure/adapters/calendar_storage_adapter.rs +++ b/src/infrastructure/adapters/calendar_storage_adapter.rs @@ -39,6 +39,14 @@ impl CalendarStorageAdapter { event_repository, } } + + /// Delegates to [`CalendarPgRepository::has_owned_calendar`] — the + /// `EXISTS` short-circuit used by the login provisioning hook instead + /// of hydrating every owned calendar to test emptiness + /// (benches/ROUND13.md §Q2). + pub async fn has_owned_calendar(&self, owner_id: Uuid) -> Result { + self.calendar_repository.has_owned_calendar(owner_id).await + } } impl CalendarStoragePort for CalendarStorageAdapter { diff --git a/src/infrastructure/repositories/pg/address_book_pg_repository.rs b/src/infrastructure/repositories/pg/address_book_pg_repository.rs index ddb16449..cd9ead3c 100644 --- a/src/infrastructure/repositories/pg/address_book_pg_repository.rs +++ b/src/infrastructure/repositories/pg/address_book_pg_repository.rs @@ -16,6 +16,23 @@ impl AddressBookPgRepository { pub fn new(pool: Arc) -> Self { Self { pool } } + + /// `EXISTS` short-circuit for the login provisioning hook — the old + /// `get_address_books_by_owner(..).is_empty()` hydrated every owned + /// `AddressBook` row on EVERY login just to test emptiness (the ROUND9 + /// §7 COUNT→EXISTS pattern; benches/ROUND13.md §Q2). + pub async fn has_owned_address_book(&self, owner_id: Uuid) -> Result { + let exists: bool = sqlx::query_scalar( + "SELECT EXISTS(SELECT 1 FROM carddav.address_books WHERE owner_id = $1)", + ) + .bind(owner_id) + .fetch_one(&*self.pool) + .await + .map_err(|e| { + DomainError::database_error(format!("Failed to probe owned address books: {}", e)) + })?; + Ok(exists) + } } impl AddressBookRepository for AddressBookPgRepository { diff --git a/src/infrastructure/repositories/pg/calendar_pg_repository.rs b/src/infrastructure/repositories/pg/calendar_pg_repository.rs index ac68d7f5..f595e75d 100644 --- a/src/infrastructure/repositories/pg/calendar_pg_repository.rs +++ b/src/infrastructure/repositories/pg/calendar_pg_repository.rs @@ -16,6 +16,24 @@ impl CalendarPgRepository { pub fn new(pool: Arc) -> Self { Self { pool } } + + /// `EXISTS` short-circuit for the login provisioning hook, which only + /// needs to know whether the user owns ANY calendar. The old + /// `list_calendars_by_owner(..).is_empty()` hydrated every owned + /// `Calendar` row (8 cols incl. description/color TEXT) on EVERY login + /// just to test emptiness — the ROUND9 §7 `Drive::is_empty` COUNT→EXISTS + /// pattern (benches/ROUND13.md §Q2). + pub async fn has_owned_calendar(&self, owner_id: Uuid) -> CalendarRepositoryResult { + let exists: bool = + sqlx::query_scalar("SELECT EXISTS(SELECT 1 FROM caldav.calendars WHERE owner_id = $1)") + .bind(owner_id) + .fetch_one(&*self.pool) + .await + .map_err(|e| { + DomainError::database_error(format!("Failed to probe owned calendars: {}", e)) + })?; + Ok(exists) + } } impl CalendarRepository for CalendarPgRepository { diff --git a/src/infrastructure/repositories/pg/recent_items_pg_repository.rs b/src/infrastructure/repositories/pg/recent_items_pg_repository.rs index 740a5d84..d7b04a68 100644 --- a/src/infrastructure/repositories/pg/recent_items_pg_repository.rs +++ b/src/infrastructure/repositories/pg/recent_items_pg_repository.rs @@ -96,19 +96,24 @@ impl RecentItemsRepositoryPort for RecentItemsPgRepository { Ok(items) } - async fn upsert_access(&self, user_id: Uuid, item_id: &str, item_type: &str) -> Result<()> { - sqlx::query( + async fn upsert_access(&self, user_id: Uuid, item_id: &str, item_type: &str) -> Result { + // `xmax = 0` on the affected row is the canonical upsert idiom for + // "this was an INSERT, not a DO UPDATE" — lets the caller skip the + // prune round-trip on the common re-access (UPDATE) path + // (benches/ROUND13.md §Q3). + let inserted: bool = sqlx::query_scalar( r#" INSERT INTO auth.user_recent_files (user_id, item_id, item_type, accessed_at) VALUES ($1, $2, $3, CURRENT_TIMESTAMP) ON CONFLICT (user_id, item_id, item_type) DO UPDATE SET accessed_at = CURRENT_TIMESTAMP + RETURNING (xmax = 0) "#, ) .bind(user_id) .bind(item_id) .bind(item_type) - .execute(&*self.db_pool) + .fetch_one(&*self.db_pool) .await .map_err(|e| { error!("Database error upserting recent item access: {}", e); @@ -119,7 +124,7 @@ impl RecentItemsRepositoryPort for RecentItemsPgRepository { ) })?; - Ok(()) + Ok(inserted) } async fn remove_item(&self, user_id: Uuid, item_id: &str, item_type: &str) -> Result { diff --git a/src/infrastructure/repositories/pg/user_pg_repository.rs b/src/infrastructure/repositories/pg/user_pg_repository.rs index 90d05a5d..ae216446 100644 --- a/src/infrastructure/repositories/pg/user_pg_repository.rs +++ b/src/infrastructure/repositories/pg/user_pg_repository.rs @@ -414,6 +414,16 @@ impl UserRepository for UserPgRepository { /// recipient expansion). Missing ids are silently skipped — the /// caller treats absent rows as "no such recipient", same as /// `get_user_by_id` returning `NotFound` for a single lookup. + /// + /// Notification-recipient projection: the up-to-512 KiB avatar `image` + /// and the `ui_preferences` JSONB are NOT hydrated (both come back as + /// `None`/`Null`) — the sole caller + /// (`RecipientNotificationService`) reads only the email/eligibility + /// fields, and a group fan-out of M members otherwise detoasted + + /// shipped + parsed M avatars purely to discard them (the ROUND12 §Q1 + /// avatar-narrowing pattern; benches/ROUND13.md §Q1). If a future + /// caller needs the avatar, add a wide sibling rather than widening + /// this one back. async fn get_users_by_ids(&self, ids: Vec) -> UserRepositoryResult> { if ids.is_empty() { return Ok(Vec::new()); @@ -425,9 +435,8 @@ impl UserRepository for UserPgRepository { id, username, email, password_hash, role::text as role_text, storage_quota_bytes, storage_used_bytes, created_at, updated_at, last_login_at, active, - oidc_provider, oidc_subject, image, is_external, - given_name, family_name, email_verified_at, preferred_locale, notify_on_share, - ui_preferences + oidc_provider, oidc_subject, is_external, + given_name, family_name, email_verified_at, preferred_locale, notify_on_share FROM auth.users WHERE id = ANY($1) "#, @@ -460,14 +469,14 @@ impl UserRepository for UserPgRepository { row.get("active"), row.get("oidc_provider"), row.get("oidc_subject"), - row.get("image"), + None, // image — not projected (notification-recipient path) row.get("is_external"), row.get("given_name"), row.get("family_name"), row.get("email_verified_at"), row.get("preferred_locale"), row.get("notify_on_share"), - row.get::("ui_preferences"), + serde_json::Value::Null, // ui_preferences — not projected ) }) .collect()) diff --git a/src/interfaces/api/routes.rs b/src/interfaces/api/routes.rs index cc2ad3e0..1d113ac6 100644 --- a/src/interfaces/api/routes.rs +++ b/src/interfaces/api/routes.rs @@ -10,7 +10,6 @@ use axum::{ }; use serde_json::json; use std::sync::Arc; -use tower_http::trace::TraceLayer; use utoipa::OpenApi; /// Liveness probe — returns 200 if the process is running, no DB check. @@ -672,12 +671,14 @@ pub fn create_api_routes(app_state: &Arc) -> Router> { // them on every overlapping request. router = router.route("/{*rest}", any(api_not_found)); - // Compression is applied once, globally, in `main.rs` with a content-type - // aware predicate that skips already-compressed media. Re-applying it here - // would double-wrap `/api`: this inner layer (no predicate) would compress - // media downloads, burning CPU for ~0 gain and stripping `Content-Length`. - // So this router only adds tracing; compression is the global layer's job. - router.layer(TraceLayer::new_for_http()) + // No per-router layers: the global `TraceLayer` + request-id stack in + // `main.rs` wraps the whole app (this `/api` router is nested into it), + // so a second `TraceLayer` here just double-wrapped every `/api` + // request in a redundant span + response-future poll (benches/ROUND13.md + // §H1). Compression is likewise the global layer's job — re-applying it + // here (no predicate) would compress media downloads, burning CPU for + // ~0 gain and stripping `Content-Length`. + router } /// Catch-all 404 for unknown `/api/*` paths. Pure log-anchoring diff --git a/src/interfaces/middleware/locale.rs b/src/interfaces/middleware/locale.rs index b632f766..d3ed5bf5 100644 --- a/src/interfaces/middleware/locale.rs +++ b/src/interfaces/middleware/locale.rs @@ -64,9 +64,15 @@ impl FromRequestParts> for RequestLocale { .get(axum::http::header::ACCEPT_LANGUAGE) .and_then(|v| v.to_str().ok()) { - let supported_owned: Vec = - registry.iter().map(|l| l.as_str().to_string()).collect(); - let supported: Vec<&str> = supported_owned.iter().map(String::as_str).collect(); + // Borrow the precomputed supported-codes list (materialized + // once at registry build) instead of rebuilding N heap Strings + // per anonymous request (benches/ROUND13.md §L1). Only the + // `&[&str]` view the crate needs is built here. + let supported: Vec<&str> = registry + .supported_codes() + .iter() + .map(String::as_str) + .collect(); if let Some(matched) = accept_language::intersection(header_value, &supported).first() && let Some(locale) = registry.parse(matched) { diff --git a/src/interfaces/middleware/trace_span.rs b/src/interfaces/middleware/trace_span.rs index bca000aa..34a470d7 100644 --- a/src/interfaces/middleware/trace_span.rs +++ b/src/interfaces/middleware/trace_span.rs @@ -92,7 +92,11 @@ pub struct ClientIpMakeSpan; impl MakeSpan for ClientIpMakeSpan { fn make_span(&mut self, request: &axum::http::Request) -> Span { - let ip = super::trusted_proxy::client_ip(request, true); + // Borrow-only IP resolution: the span records `client_ip` via `%ip` + // (Display), so a `ClientIpDisplay` that renders straight into the + // span's field storage avoids the per-request `String` the owned + // `client_ip()` allocated (benches/ROUND13.md §H2). + let ip = super::trusted_proxy::client_ip_display(request, true); let request_id = request .headers() .get("x-request-id") diff --git a/src/interfaces/middleware/trusted_proxy.rs b/src/interfaces/middleware/trusted_proxy.rs index d6bb6310..110c2525 100644 --- a/src/interfaces/middleware/trusted_proxy.rs +++ b/src/interfaces/middleware/trusted_proxy.rs @@ -146,6 +146,82 @@ pub fn client_ip(req: &Request, include_port: bool) -> String { client_ip_from_parts(req.headers(), peer, include_port) } +/// A resolved client-IP source that borrows from the request instead of +/// allocating a `String`. [`std::fmt::Display`] renders it directly into the +/// caller's buffer (the tracing span's field storage), so the per-request +/// span factory no longer materializes an intermediate `String` on every +/// request (benches/ROUND13.md §H2). Bytes rendered are identical to +/// [`client_ip`]/[`client_ip_from_parts`] for all four cases. +pub enum ClientIpDisplay<'a> { + /// Proxy-forwarded client address (borrowed from `X-Forwarded-For` / + /// `X-Real-Ip`), already trimmed. + Forwarded(&'a str), + /// Direct TCP peer, rendered with the port. + PeerWithPort(SocketAddr), + /// Direct TCP peer, rendered as the bare IP. + PeerIp(IpAddr), + /// No connection info available. + Unknown, +} + +impl std::fmt::Display for ClientIpDisplay<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ClientIpDisplay::Forwarded(s) => f.write_str(s), + ClientIpDisplay::PeerWithPort(addr) => write!(f, "{addr}"), + ClientIpDisplay::PeerIp(ip) => write!(f, "{ip}"), + ClientIpDisplay::Unknown => f.write_str("unknown"), + } + } +} + +/// Zero-allocation twin of [`client_ip_from_parts`]: resolves the client-IP +/// source without producing an owned `String`. The returned value borrows +/// `headers`, so it must be `Display`-rendered before `headers` is dropped +/// (the span factory does this synchronously). +pub fn client_ip_display_from_parts<'a>( + headers: &'a axum::http::HeaderMap, + peer: Option, + include_port: bool, +) -> ClientIpDisplay<'a> { + if let Some(peer_addr) = peer { + if is_trusted_proxy(peer_addr.ip()) { + if let Some(xff) = headers.get("x-forwarded-for").and_then(|v| v.to_str().ok()) + && let Some(ip) = xff + .split(',') + .next() + .map(str::trim) + .filter(|s| !s.is_empty()) + { + return ClientIpDisplay::Forwarded(ip); + } + if let Some(xri) = headers + .get("x-real-ip") + .and_then(|v| v.to_str().ok()) + .map(str::trim) + .filter(|s| !s.is_empty()) + { + return ClientIpDisplay::Forwarded(xri); + } + } + return if include_port { + ClientIpDisplay::PeerWithPort(peer_addr) + } else { + ClientIpDisplay::PeerIp(peer_addr.ip()) + }; + } + ClientIpDisplay::Unknown +} + +/// Zero-allocation twin of [`client_ip`] for the request-span factory. +pub fn client_ip_display(req: &Request, include_port: bool) -> ClientIpDisplay<'_> { + let peer: Option = req + .extensions() + .get::>() + .map(|ci| ci.0); + client_ip_display_from_parts(req.headers(), peer, include_port) +} + /// Same as [`client_ip`], but operates on already-extracted parts (headers /// plus an optional TCP peer). Handlers that don't take a full `Request`, /// e.g. those that consume the body via `Json<…>`, can still derive a stable